Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36971 - GUI to allow cloning of Ansible roles from VCS #676

Closed

Conversation

Thorben-D
Copy link
Contributor

@Thorben-D Thorben-D commented Dec 8, 2023

This feature adds a new modal to /ansible/ansible_roles, which allows the user to clone a git-repo with roles to a specified SmartProxy.

RedMine-Ticket

GUI:

Screenshot_20231208_134233

Depends on theforeman/smart_proxy_ansible#85

@Thorben-D Thorben-D changed the title Fixes #36971 - GUI to allow cloning of ansible-roles from VCS Fixes #36971 - GUI to allow cloning of Ansible roles from VCS Dec 8, 2023
@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from e54414f to d86438d Compare December 8, 2023 14:44
@nofaralfasi
Copy link
Contributor

I'll test it again after the tests are successful because it is not working for me now.

@Thorben-D
Copy link
Contributor Author

I fixed the linting issues and retested everything.
Tests are running fine on my machine.

@nofaralfasi
Copy link
Contributor

I'm getting the following error from the GUI:

Performing CloneAnsibleRole (Job ID: 311cb572-ffe0-43c1-be85-3253315eb4d3) from Dynflow(default) enqueued at 2023-12-14T10:15:52Z with arguments: {"vcs_url"=>"https://github.com/theforeman/foreman.git", "name"=>"theforeman.foreman", "ref"=>"", "update"=>false}, #<GlobalID:0x00007fb5c0bda290 @uri=#<URI::GID gid://foreman/SmartProxy/1>>
Error performing CloneAnsibleRole (Job ID: 311cb572-ffe0-43c1-be85-3253315eb4d3) from Dynflow(default) in 65.03ms: Foreman::Exception (ERF42-7661 [Foreman::Exception]: »Git Error. Check log.«):

@Thorben-D
Copy link
Contributor Author

I added the missing permissions. The pipeline should now complete.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really wise to have Foreman use git directly? If so, you should consider HTTP proxies too.

I would implement this in the Smart Proxy and expose it via the Smart Proxy API. Otherwise foreman_ansible gets a dependency on git. Credentials will also be challenging because they need to be used in two places now.

This will be a longer story, so bear with me.

Foreman has traditionally tried to avoid doing anything on Foreman itself. It only really runs things on a Smart Proxy. There is also no requirement at all to run Foreman and Smart Proxy on the same system. So they share nothing, except communication via HTTP (REST) APIs.

If you implement git operations on Foreman, they are less reliable. For example, if you have a git server with a firewall then you will first allow the Foreman server itself. Then it can list the info, but if you actually clone it to a Smart Proxy then it may still fail. If you only implement it using a Smart Proxy then listing is a valid check if a clone can proceed.

You should also implement a capability in the Smart Proxy to indicate it can clone and make sure it does support that. https://theforeman.org/2019/04/smart-proxy-capabilities-explained.html is what I wrote about it back when I introduced it. Foreman also has APIs like has_capability? (see suggestion inline for that as well). Doing this properly is critical because we have n-1 support, which means Foreman x.y should be able to work with a Foreman Proxy x.(y-1) which won't have the capability to manage VCS. With the capabilities framework you can detect this using the information in the Foreman DB and properly inform the user. Either by not listing the proxies lacking the capability or displaying them but warning the user about the missing capability.

Once you implement everything via a Smart Proxy then it probably makes sense to make it part of the smart_proxies REST API collection.

app/controllers/api/v2/vcs_clone_controller.rb Outdated Show resolved Hide resolved
app/jobs/clone_ansible_role.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/vcs_clone_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/vcs_clone_controller.rb Outdated Show resolved Hide resolved
config/routes.rb Outdated Show resolved Hide resolved
foreman_ansible.gemspec Outdated Show resolved Hide resolved
app/controllers/api/v2/vcs_clone_controller.rb Outdated Show resolved Hide resolved
app/controllers/api/v2/vcs_clone_controller.rb Outdated Show resolved Hide resolved
@Thorben-D
Copy link
Contributor Author

I forgot to remove the 'git' dependency and foolishly force pushed again after removing it.
That kind of broke the 'Compare' function...
The actual commit that addresses the feedback from @ekohl is ae45011

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing my concerns. I've only looked at the Ruby side of it, not the front end but I think this generally looks good.

I didn't see it in the PR, but for myself: this relies on theforeman/smart_proxy_ansible#85.

app/controllers/api/v2/vcs_clone_controller.rb Outdated Show resolved Hide resolved
app/lib/proxy_api/ansible.rb Outdated Show resolved Hide resolved
Comment on lines +59 to +60
rescue *PROXY_ERRORS, RestClient::Exception => e
raise e unless e.is_a? RestClient::RequestFailed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd only catch the specific exception:

Suggested change
rescue *PROXY_ERRORS, RestClient::Exception => e
raise e unless e.is_a? RestClient::RequestFailed
rescue RestClient::RequestFailed => e

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had something different in my mind, but just now realized that your suggestion works fine and is cleaner...
I'll add it after the new changes have been reviewed.

Comment on lines 54 to 53
api :PUT, '/smart_proxies/:proxy_name/ansible/vcs_clone/roles',
N_('Launches a task to update the provided role')
formats ['json']
param :repo_info, Hash, :desc => N_('Dictionary containing info about the role to be installed') do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantically I'd expect a PUT on a collection to ensure that the provided list of items is the exact set of items present. Or put in other words, PUT on .../roles with my_role would ensure that only my_role is present and all other roles are removed.

If the name is immutable (and I think it's safe to assume so) you could make it part of the URL and make it PUT .../roles/:name to update things (like VCS URL and ref). Just like you did with DELETE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added role_name as a parameter to the URL, which does indeed make more sense now!

@Thorben-D
Copy link
Contributor Author

The latest commit splits up the large React-components into smaller, more manageable ones. The Ruby-part stays untouched.

@gardar
Copy link
Contributor

gardar commented Feb 9, 2024

Excited to see some git integration being added for foreman_ansible, but I have few thoughts.

  • How about syncing collections too? Ansible is moving into the direction of collections and away from standalone roles.
  • Wouldn't it have been better to do the integration with ansible-galaxy? ansible-galaxy can clone both roles and collections from git but also fetch them from a galaxy server.

With that being said, do you have any plans to add further git integration with foreman_ansible? Would love to be able to sync the ansible variables to and from a git repo https://projects.theforeman.org/issues/36770

@OttaviaB
Copy link

That is a good idea about collections!
In foreman, collections are still handled in a fundamentally different way than roles though, right?
Roles are imported and assigned to hosts/host groups, while collections must be simply installed once and (at least in my experience) are usually not updated as often as roles.
I guess that the main pain point is with roles for now, but it definetely makes sense to think about collections as well in the future.

Using ansible-galaxy instead of git is a great suggestion. Maybe we can do that later?

@gardar
Copy link
Contributor

gardar commented Feb 23, 2024

In foreman, collections are still handled in a fundamentally different way than roles though, right?

Yes and no, a collection can be a collection of roles or even a single role. The collection can also include ansible modules, playbooks, etc. All of this is optional so that the collections don't have to contain roles but they also don't have to contain modules, etc.

Roles are imported and assigned to hosts/host groups, while collections must be simply installed once and (at least in my experience) are usually not updated as often as roles.

By installing a collection that contains roles on a foreman server you get ansible roles that you can import in foreman and can then assign those to hosts/host groups.

It's absolute nonsense that collections are usually not updated as often as roles, especially when collections can include roles.

When roles are packaged in a collection you can't install just the roles unless you manually copy the roles out of the collection which is something that should be avoided at all cost.
I think it's more trouble than it's worth to do this without using ansible-galaxy as ansible-galaxy already handles the roles and collections correctly.

@Thorben-D
Copy link
Contributor Author

@nofaralfasi
Thanks for taking a look!
I fixed the issue with the modal opening automatically and the button being inconsistent.
I also fixed an issue that was causing the modal to crash. I'm not sure though if that was the one you encountered though...

@nofaralfasi
Copy link
Contributor

Unfortunately, this is still not working for me as accepted.
I've messaged @Thorben-D the error I'm encountering. Please advise on how we should proceed from here.

@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 91d98e0 to 01558dd Compare March 19, 2024 07:52
@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch 2 times, most recently from 91877c5 to dae9172 Compare March 26, 2024 10:57
@nofaralfasi
Copy link
Contributor

Here are some suggested improvements to the GUI:

  1. Enhance the GUI by including an asterisk next to the mandatory fields.
  2. When clicking on the Examine button with a non-existing Git link (in the correct format), the following issues occur:
    • No error message is displayed.
    • The application becomes unresponsive and requires a restart to function properly again.
  3. Clicking on the Confirm button does not close the modal upon successful completion.
  4. Enhance the form by adding an icon featuring a question mark, providing additional details about each field when hovered over.

@Thorben-D
Copy link
Contributor Author

I have made the following changes:

  • Mark required fields
  • Add help icons to fields
  • Fix a bug that caused the "Examine" button to break
  • Make the modal close upon completion
  • Change "SmartProxy" to "Smart Proxy"
  • Change default ref from "master" to "main"
  • Remove "Final task" component
  • Rename "Examine" button to "Load metadata"
  • Change "Repo name" to "Role name"
  • Change Smart Proxy selector type

Unfortunately, I deleted one thing too many and broke the UI. Pushing the fix broke the "Compare" button.
Please use this to compare changes.

@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from d01402a to f5445ce Compare April 8, 2024 07:27
@nofaralfasi
Copy link
Contributor

Thanks, @Thorben-D, for the improvements!

Other things I'm noticing:

  • The tabs appear disabled even when they are enabled.
  • The "SmartProxy required" message should be placed under the smart proxy selection.
  • The blue border around the "Collision handling" switch is unnecessary. Additionally, I believe the text of the switch should remain "Update existing" and only the switch should be toggled on/off.

Renaming suggestions:

  • Rename "Smart Proxy name" to "Choose Smart Proxy"
  • Rename "Import from VCS" to "Clone from Git"
  • Rename "Get Ansible roles from VCS" to "Clone Ansible roles from Git"
  • "When toggled, Foreman will override already installed roles with the given name." to "When toggled, Foreman will overwrite existing roles with the same name."

@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from f5445ce to 4cb6370 Compare April 9, 2024 07:59
Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some actions, such as listing and deleting, are not directly related to the ability to import Ansible roles from Git. Perhaps I'm overthinking it, but I suggest we should decide whether to move these actions to the AnsibleRolesController or keep the structure as is.

app/jobs/clone_ansible_role.rb Outdated Show resolved Hide resolved
app/jobs/delete_ansible_role.rb Outdated Show resolved Hide resolved
@@ -31,6 +31,12 @@ def ansible_proxy_import(hash)
ansible_proxy_links(hash))
end

def vcs_import
select_action_button("",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not display this button if the user does not have the required permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this as well. Will add later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I am not sure how to handle this... Ideally, this would be handled on the React side.

app/views/ansible_roles/index.html.erb Outdated Show resolved Hide resolved
app/views/ansible_roles/welcome.html.erb Show resolved Hide resolved
@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 4cb6370 to 9a79824 Compare April 30, 2024 13:11
@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch 2 times, most recently from f67d9b8 to be5b27a Compare May 3, 2024 09:39
@Thorben-D Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from be5b27a to 04b2e91 Compare May 3, 2024 10:27

before_action :set_proxy_api

api :GET, '/smart_proxies/:smart_proxy_id/ansible/vcs_clone/repository_metadata',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
api :GET, '/smart_proxies/:smart_proxy_id/ansible/vcs_clone/repository_metadata',
api :GET, '/smart_proxies/:smart_proxy_id/vcs_clone/repository_metadata',

There is no need to include ansible in the url path. IMHO, adding the api_base_url '/ansible/api', is enough.

Comment on lines 32 to 102
<Tab
data-testid="BranchTagSelectionMenuManualTab"
eventKey={0}
title={<TabTitleText>{__('Manual input')}</TabTitleText>}
>
<Form>
<FormGroup
label={__('Branch / Tag / Commit')}
labelIcon={
<Popover
headerContent={
<div>
{__('Enter a valid')}{' '}
<a
href="https://git-scm.com/book/en/v2/Git-Internals-Git-References"
target="_blank"
rel="noreferrer"
>
{__('Git reference')}
</a>
.
</div>
}
>
<button
type="button"
aria-label="More info for ref selection field"
onClick={e => e.preventDefault()}
aria-describedby="simple-form-name-01"
className="pf-c-form__group-label-help"
>
<HelpIcon noVerticalAlign />
</button>
</Popover>
}
isRequired
>
<TextInput
id="branch_tag_commit_input"
value={props.gitRef}
onChange={value => props.setGitRef(value)}
/>
</FormGroup>
</Form>
</Tab>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the UI after the changes now, and I think the previous version looked better.
The default selected tab should be the rightmost one.

@nofaralfasi
Copy link
Contributor

Since this PR has been inactive for a while, we’re closing it to keep the repository organized. If you’d like to continue working on it, please feel free to reopen or create a new PR.

@nofaralfasi nofaralfasi closed this Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants