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

Thorben-D · 2023-12-08T15:00:36Z

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

Depends on theforeman/foreman_ansible#676

theforeman-bot · 2023-12-08T15:00:40Z

Can one of the admins verify this patch?

test/vcs_cloner_test.rb

Thorben-D · 2023-12-29T15:35:00Z

I just pushed a commit that:

Moves all Git-functionality to this plugin, letting me remove the git dependency from the foreman plugin.
Improves code readability by splitting up big functions into a helper-file
Improves the tests

ekohl

In smart-proxy plugins you can also use load_programmable_settings to dynamically perform logic on it. https://github.com/theforeman/smart-proxy/blob/2fb6db34f8726df3e8b276c4ced0542fd296bc6a/modules/httpboot/httpboot_plugin.rb#L7-L11 is one example.

What you can do with that is define one path which contains the mutable roles. Then you have another setting which contains all the paths that are loaded.

You can expose this to the user in two ways.

One is explicit: there are 2 settings and in the plugin definition you make sure the mutable roles path is in the readable roles path list.

The other is implicit: you only have a single list of role paths and take the first item of that list as the mutable roles path.

On start up you can make sure the mutable roles directory exists or can be created. You can even only expose the vcs_clone capability if it's working, but I'd be cautious with that because we don't refresh features all the time so you can easily end up with a hard to debug situation. theforeman/smart-proxy#847 is IMHO the better way to go with such health checks.

ekohl · 2023-12-31T14:22:53Z

lib/smart_proxy_ansible/api.rb

+      get '/vcs_clone/get_installed' do
+        get_installed = VCSCloner.list_installed_roles
+        status get_installed.status
+        body get_installed.payload.to_json
+      end
+
+      post '/vcs_clone/install' do
+        install = VCSCloner.install(params['repo_info'])
+        status install.status
+        body install.payload.to_json
+      end
+
+      put '/vcs_clone/update' do
+        update = VCSCloner.update(params['repo_info'])
+        status update.status
+        body update.payload.to_json
+      end
+
+      delete '/vcs_clone/delete/:role_name' do


Similar to my comments on the Foreman PR side (theforeman/foreman_ansible#676), I'd make it a bit more REST. So:

GET /vcs_clone/roles to list

POST /vcs_clone/roles to install

PUT /vcs_clone/roles/:name to update

DELETE /vcs_clone/roles/:name to delete

Again, thank you very much for the detailed feedback. Very sorry I forgot to cross-link MRs. Glad you found it anyways!
I added settings for the roles paths am not using plain strings anymore.
Currently, an exception is thrown if the mutable-roles path does not exist or is not writable (and the feature is enabled), effectively disabling the plugin.
I do however think the verification-API is a good idea. Maybe we could integrate this into the existing /features route? Somethink like /features/active might be a good idea...

ekohl · 2023-12-31T14:25:45Z

lib/smart_proxy_ansible/vcs_cloner_helper.rb

+          %w[vcs_url name ref].each do |param|
+            return false unless repo_info.key?(param)
+          end


Suggested change

%w[vcs_url name ref].each do |param|

return false unless repo_info.key?(param)

end

%w[vcs_url name ref].all? { |param| repo_info.key?(param) }

ekohl · 2023-12-31T14:28:58Z

lib/smart_proxy_ansible/roles_reader.rb

    class RolesReader
      class << self
-        DEFAULT_ROLES_PATH = '/etc/ansible/roles:/usr/share/ansible/roles'.freeze
+        DEFAULT_ROLES_PATH = '/etc/ansible/roles:/usr/share/ansible/roles:/var/lib/foreman-proxy/ansible/roles'.freeze


I'd think this should read first from the mutable directory so VCS cloned repos can override system wide repos. See the overall review for more on settings.

ekohl · 2023-12-31T14:29:32Z

lib/smart_proxy_ansible/vcs_cloner_helper.rb

+    # Implements VCS-Cloning logic and helper functions
+    class VcsClonerHelper
+      class << self
+        DEFAULT_BASE_PATH = Pathname('/var/lib/foreman-proxy/ansible/roles')


I'd prefer settings. In lib/smart_proxy_ansible/plugin.rb you see a list of default_settings and you can provide it there. See the overall review for more on settings.

ekohl

Not a complete review, just focusing on one item.

ekohl · 2024-01-06T14:55:45Z

lib/smart_proxy_ansible/plugin.rb

+        mutable_roles_path = '/var/lib/foreman-proxy/ansible/roles'
+        system_roles_path = '/etc/ansible/roles:/usr/share/ansible/roles'


These should be settings because in development environments or containers you may want to use different paths.

I've been thinking about some constants in the Proxy module, but the other day I realized that the Proxy::Plugin API is an even better place. That can combine those constants with the specific plugin name. So you could get a state_dir(path) method that would evaluate state_dir('roles') to /var/lib/foreman-proxy/ansible/roles in production. I thought I had some PR for that, but can't find the code I wrote. Essentially, I'd base it on the table in https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#RuntimeDirectory=

I'd very much welcome PRs that implement this. It's a recurring problem in plugins.

I have now added these fields as arguments that may be passed in the configuration file.
The mutable_roles_path is constructed like this: StateDirectory/plugin_name/roles
I also added runtime_, state_, cache_, logs_ and config_dir methods. Those just construct the respective paths from an environment-variable and the plugin name. The env-variables conform to the systemd table you linked, so they can be passed easily.
If this is what you had in mind, I could create a merge-request to merge the xxxx_dir-methods into smart-proxy/lib/proxy/plugin.rb for reusability.

lib/smart_proxy_ansible/plugin.rb

nofaralfasi · 2024-03-17T15:52:42Z

I'm getting the following error:
Disabling all modules in the group ['ansible'] due to a failure in one of them: /var/lib/foreman-proxy/ansible/roles is not writable. Check permissions or disable vcs_integration
Maybe that's something we need to address in the documentation?

nofaralfasi · 2024-03-27T10:59:44Z

settings.d/ansible.yml.example

 ---
 :enabled: true
 :working_dir: '~/.foreman-ansible'
+:vcs_integration: true


When setting :vcs_integration: false, I can still trigger the role cloning from the GUI. However, the task remains stuck in the pending status.
I believe the intended behavior here is to restrict access to the new form, correct?

Yes, that would be ideal but iirc, the functionality to inform Foreman of enabled/disabled features is not yet a thing as described in the last paragraph of @ekohl's comment here:
#85 (review)

nofaralfasi · 2024-03-28T09:53:50Z

settings.d/ansible.yml.example

+:static_roles_paths: [
+  '/etc/ansible/roles',
+  '/usr/share/ansible/roles',
+]


Does this work for you?
I have a different location where I store my Ansible roles, and setting it here doesn't work for me.

Are your paths relative perhaps? I just tested it and it works fine with absolute paths, but not relative ones.

My path is /home/nalfassi/playbooks. It's also configured in my /etc/ansible/ansible.cfg as follows:
roles_path = /home/nalfassi/playbooks.

And I'm using my localhost as my smart-proxy.

nofaralfasi · 2024-09-02T10:44:06Z

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.

pr-processor bot added the Not yet reviewed label Dec 8, 2023

nofaralfasi reviewed Dec 14, 2023

View reviewed changes

test/vcs_cloner_test.rb Outdated Show resolved Hide resolved

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from ade48e3 to 6a7c5fd Compare December 29, 2023 14:48

ekohl mentioned this pull request Dec 31, 2023

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

Closed

ekohl requested changes Dec 31, 2023

View reviewed changes

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

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 6a7c5fd to 3905126 Compare January 5, 2024 12:57

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

ekohl reviewed Jan 6, 2024

View reviewed changes

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 3905126 to 65cb4b6 Compare January 9, 2024 13:50

nofaralfasi reviewed Mar 3, 2024

View reviewed changes

lib/smart_proxy_ansible/plugin.rb Outdated Show resolved Hide resolved

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 65cb4b6 to 32eccb7 Compare March 8, 2024 15:08

nofaralfasi reviewed Mar 27, 2024

View reviewed changes

nofaralfasi reviewed Mar 28, 2024

View reviewed changes

Thorben-D mentioned this pull request Mar 28, 2024

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

Closed

1 task

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

443d48b

Thorben-D force-pushed the issue/ORA-338_vcs_role_import branch from 32eccb7 to 443d48b Compare April 9, 2024 09:24

nofaralfasi closed this Sep 2, 2024

		mutable_roles_path = '/var/lib/foreman-proxy/ansible/roles'
		system_roles_path = '/etc/ansible/roles:/usr/share/ansible/roles'

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

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

Uh oh!

Conversation

Thorben-D commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theforeman-bot commented Dec 8, 2023

Uh oh!

Uh oh!

Thorben-D commented Dec 29, 2023

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekohl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thorben-D Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nofaralfasi commented Mar 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nofaralfasi commented Sep 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Thorben-D commented Dec 8, 2023 •

edited

Loading

Thorben-D Jan 9, 2024 •

edited

Loading