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

Thorben-D · 2023-12-08T13:22:09Z

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:

Depends on theforeman/smart_proxy_ansible#85

nofaralfasi · 2023-12-12T15:55:54Z

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

Thorben-D · 2023-12-13T15:01:33Z

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

nofaralfasi · 2023-12-14T10:21:57Z

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 · 2023-12-14T14:17:28Z

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

ekohl

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

app/jobs/clone_ansible_role.rb

app/controllers/api/v2/vcs_clone_controller.rb

config/routes.rb

foreman_ansible.gemspec

app/controllers/api/v2/vcs_clone_controller.rb

Thorben-D · 2023-12-30T15:31:23Z

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

ekohl

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

app/lib/proxy_api/ansible.rb

ekohl · 2023-12-31T14:11:26Z

app/lib/proxy_api/ansible.rb

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


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

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.

ekohl · 2023-12-31T14:15:31Z

app/controllers/api/v2/vcs_clone_controller.rb

+      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


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.

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

Thorben-D · 2024-01-19T14:28:08Z

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

gardar · 2024-02-09T19:17:55Z

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 · 2024-02-23T14:17:41Z

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 · 2024-02-23T15:07:01Z

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.

nofaralfasi

I'm encountering some issues when I navigate to the ansible/ansible_roles page in the UI. The new form automatically appears when I navigate to this page (without pressing anything), and the Import from VCS... button doesn't always work.
In addition, when I enter a URL and select a smart-proxy, clicking on the Examin button suddenly closes the popup form.

Thorben-D · 2024-03-08T15:11:14Z

@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 · 2024-03-17T16:15:44Z

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.

app/lib/proxy_api/ansible.rb

nofaralfasi · 2024-03-27T11:17:11Z

Here are some suggested improvements to the GUI:

Enhance the GUI by including an asterisk next to the mandatory fields.
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.
Clicking on the Confirm button does not close the modal upon successful completion.
Enhance the form by adding an icon featuring a question mark, providing additional details about each field when hovered over.

Thorben-D · 2024-03-28T14:59:31Z

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.

nofaralfasi · 2024-04-08T16:16:36Z

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."

nofaralfasi

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.

webpack/components/VcsCloneModalContent/VcsCloneModalContent.js

webpack/components/VcsCloneModalContent/components/BranchTagSelectionMenu.js

webpack/components/VcsCloneModalContent/components/ModalConfirmButton.js

webpack/components/VcsCloneModalContent/VcsCloneModalContent.js

app/jobs/clone_ansible_role.rb

webpack/components/VcsCloneModalContent/components/SmartProxySelector.js

webpack/components/VcsCloneModalContent/components/GitLinkInputComponent.js

webpack/components/VcsCloneModalContent/components/UpdateExistingSwitch.scss

app/jobs/delete_ansible_role.rb

nofaralfasi · 2024-04-21T13:01:13Z

app/helpers/foreman_ansible/ansible_roles_helper.rb

@@ -31,6 +31,12 @@ def ansible_proxy_import(hash)
                           ansible_proxy_links(hash))
    end

+    def vcs_import
+      select_action_button("",


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

Missed this as well. Will add later.

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

app/views/ansible_roles/welcome.html.erb

nofaralfasi · 2024-04-30T13:59:37Z

app/controllers/api/v2/vcs_clone_controller.rb

+
+      before_action :set_proxy_api
+
+      api :GET, '/smart_proxies/:smart_proxy_id/ansible/vcs_clone/repository_metadata',


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.

nofaralfasi · 2024-05-06T12:31:54Z

webpack/components/VcsCloneModalContent/components/BranchTagSelectionMenu.js

+      <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>


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.

pr-processor bot added Not yet reviewed Waiting on contributor labels Dec 8, 2023

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 force-pushed the issue/ORA-338_vcs_role_import branch from e54414f to d86438d Compare December 8, 2023 14:44

pr-processor bot removed the Waiting on contributor label Dec 8, 2023

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from d86438d to 9444819 Compare December 13, 2023 14:46

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 9444819 to 2870271 Compare December 14, 2023 14:15

ekohl requested changes Dec 18, 2023

View reviewed changes

pr-processor bot added Waiting on contributor and removed Not yet reviewed labels Dec 18, 2023

pr-processor bot added Needs re-review and removed Waiting on contributor labels Dec 29, 2023

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from ae45011 to 29f9de0 Compare December 29, 2023 15:23

ekohl requested changes Dec 31, 2023

View reviewed changes

pr-processor bot added Waiting on contributor and removed Needs re-review labels Dec 31, 2023

ekohl mentioned this pull request Dec 31, 2023

Fixes #36971 - GUI to allow cloning of Ansible roles from VCS theforeman/smart_proxy_ansible#85

Open

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 29f9de0 to 62813dd Compare January 5, 2024 12:58

pr-processor bot added Needs re-review and removed Waiting on contributor labels Jan 5, 2024

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 62813dd to 6832123 Compare January 19, 2024 14:26

nofaralfasi reviewed Mar 3, 2024

View reviewed changes

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 6832123 to 91d98e0 Compare March 8, 2024 15:06

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 91d98e0 to 01558dd Compare March 19, 2024 07:52

nofaralfasi reviewed Mar 20, 2024

View reviewed changes

app/lib/proxy_api/ansible.rb Outdated Show resolved Hide resolved

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

Thorben-D mentioned this pull request Mar 28, 2024

Document VCS integration in foreman_ansible plugin theforeman/foreman-documentation#2920

Draft

1 task

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch 2 times, most recently from d375814 to d01402a Compare March 28, 2024 14:50

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

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

nofaralfasi requested changes Apr 21, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Needs re-review labels Apr 21, 2024

MariaAga suggested changes Apr 25, 2024

View reviewed changes

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 force-pushed the issue/ORA-338_vcs_role_import branch from 4cb6370 to 9a79824 Compare April 30, 2024 13:11

pr-processor bot added Needs re-review and removed Waiting on contributor labels Apr 30, 2024

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

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

04b2e91

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from be5b27a to 04b2e91 Compare May 3, 2024 10:27

nofaralfasi requested changes May 6, 2024

View reviewed changes

pr-processor bot added Waiting on contributor and removed Needs re-review labels May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Thorben-D commented Dec 8, 2023 •

edited

nofaralfasi commented Dec 12, 2023

Thorben-D commented Dec 13, 2023

nofaralfasi commented Dec 14, 2023

Thorben-D commented Dec 14, 2023

ekohl left a comment

Thorben-D commented Dec 30, 2023

ekohl left a comment

ekohl Dec 31, 2023

Thorben-D Jan 5, 2024

ekohl Dec 31, 2023

Thorben-D Jan 5, 2024

Thorben-D commented Jan 19, 2024

gardar commented Feb 9, 2024 •

edited

OttaviaB commented Feb 23, 2024

gardar commented Feb 23, 2024

nofaralfasi left a comment

Thorben-D commented Mar 8, 2024

nofaralfasi commented Mar 17, 2024

nofaralfasi commented Mar 27, 2024

Thorben-D commented Mar 28, 2024

nofaralfasi commented Apr 8, 2024

nofaralfasi left a comment

nofaralfasi Apr 21, 2024

Thorben-D Apr 30, 2024

Thorben-D Apr 30, 2024

nofaralfasi Apr 30, 2024

nofaralfasi May 6, 2024

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

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


		before_action :set_proxy_api

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

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

Are you sure you want to change the base?

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

Conversation

Thorben-D commented Dec 8, 2023 • edited

nofaralfasi commented Dec 12, 2023

Thorben-D commented Dec 13, 2023

nofaralfasi commented Dec 14, 2023

Thorben-D commented Dec 14, 2023

ekohl left a comment

Choose a reason for hiding this comment

Thorben-D commented Dec 30, 2023

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thorben-D commented Jan 19, 2024

gardar commented Feb 9, 2024 • edited

OttaviaB commented Feb 23, 2024

gardar commented Feb 23, 2024

nofaralfasi left a comment

Choose a reason for hiding this comment

Thorben-D commented Mar 8, 2024

nofaralfasi commented Mar 17, 2024

nofaralfasi commented Mar 27, 2024

Thorben-D commented Mar 28, 2024

nofaralfasi commented Apr 8, 2024

nofaralfasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thorben-D commented Dec 8, 2023 •

edited

gardar commented Feb 9, 2024 •

edited