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

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

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

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Nov 11, 2022

This allows re-running the provisioner without overwriting the user's changes to the configuration, so that the installer in devel setups more closely matches the familiar standard behavior.

@wbclark
Copy link
Contributor Author

wbclark commented Nov 15, 2022

Test failure unrelated but #1605 should fix that

This allows re-running the provisioner without overwriting the
user's changes to the configuration, so that the installer in
devel setups more closely matches the familiar standard behavior.
@wbclark
Copy link
Contributor Author

wbclark commented Nov 17, 2022

Tests are now 🍏

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

Copy link
Member

@ehelms ehelms left a comment

Choose a reason for hiding this comment

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

Feel free to do the force as a follow up or in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants