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.

Sign up for GitHub

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

Jump to bottom

Don't overwrite katello-devel-answers.yaml #1604

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Don't overwrite katello-devel-answers.yaml #1604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1a82fc
Don't overwrite katello-devel-answers.yaml
wbclark Nov 11, 2022
09ee92c
Move dynamic values from devel answers to installer params
wbclark Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions roles/foreman_installer_devel_scenario/files/katello-devel.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
:log_name: katello-devel.log
:log_level: DEBUG
:no_prefix: false
# Forklift always overwrites the answers so let's not pretend we save them
wbclark marked this conversation as resolved.
Show resolved Hide resolved
:dont_save_answers: true
:answer_file: /etc/foreman-installer/scenarios.d/katello-devel-answers.yaml
:installer_dir: /usr/share/foreman-installer
:module_dirs: /usr/share/foreman-installer/modules
Expand Down
3 changes: 2 additions & 1 deletion roles/foreman_installer_devel_scenario/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
with_items:
- katello-devel.yaml

- name: 'Copy answers file'
- name: "Create devel scenario answers file if it doesn't already exist"
Copy link
Member

Choose a reason for hiding this comment

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

This will result in some options to this role only affecting the first run of the role and then never again -- that is generally an unexpected situation for Ansible roles.

If we are going this route, I should think we need to go all the way and make it such that the dynamic values we do have (https://github.com/theforeman/forklift/blob/master/roles/foreman_installer_devel_scenario/templates/katello-devel-answers.yaml#L3-L8) get passed to the installer as parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I pushed an additional commit which moves these dynamic values out of the answers file and into foreman_installer_options_internal_use_only, which is probably where they should have been all along. (What clown put dynamic values in the answers file? Oh right, it's me. I'm the clown.)

So, the behavior should now be consistent with what you expect from an ansible role. Updating the foreman_installer_devel_scenario_user or foreman_installer_devel_scenario_group will again properly result in the installer making the appropriate changes.

I also removed generate and deploy from the certs params in the answers file, because the values we were passing were already the defaults for that module, so I couldn't see any reason why those needed to be there. I can revert that if it's needed for any reason.

I've tested this in my own setup, which does include a non-default value for foreman_installer_devel_scenario_user and it's all working just fine for me.

template:
src: "{{ role_path }}/templates/{{ item }}"
dest: /etc/foreman-installer/scenarios.d/{{ item }}
force: no
wbclark marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ehelms would the benefit from being toggle-able? force: yes (i.e. the old behavior, which is the default when force is omitted entirely) would be the default but the user could specify instead not to overwrite with a variable like foreman_installer_devel_scenario_dont_overwrite_answers_file)

Copy link
Member

Choose a reason for hiding this comment

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

I can see that being useful.

with_items:
- katello-devel-answers.yaml

Expand Down
9 changes: 2 additions & 7 deletions roles/foreman_installer_devel_scenario/templates/katello-devel-answers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
---
certs:
group: {{ foreman_installer_devel_scenario_group | default(foreman_installer_devel_scenario_user) }}
deploy: true
generate: true
katello_devel:
deployment_dir: /home/{{ foreman_installer_devel_scenario_user }}
user: {{ foreman_installer_devel_scenario_user }}
certs: true
katello_devel: true
foreman_proxy_content:
enable_katello_agent: true
proxy_pulp_yum_to_pulpcore: true
Expand Down
3 changes: 3 additions & 0 deletions roles/katello_devel/meta/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,7 @@ dependencies:
- "--katello-devel-admin-password {{ foreman_installer_admin_password }}"
- "{{ '--katello-devel-github-username=' + katello_devel_github_username if katello_devel_github_username is defined else '' }}"
- "--katello-devel-extra-plugins theforeman/foreman_remote_execution"
- "{{ '--katello-devel-deployment-dir' }} {{ '/home/' + foreman_installer_devel_scenario_user }}"
- "{{ '--katello-devel-user' }} {{ foreman_installer_devel_scenario_user }}"
- "{{ '--certs-group' }} {{ foreman_installer_devel_scenario_group | default(foreman_installer_devel_scenario_user) }}"
- role: customize_home