Test include roles dynamically #201
Conversation
molecule/dynamic/group_vars/all
Outdated
| @@ -0,0 +1,13 @@ | |||
| --- | |||
| pulp_default_admin_password: password | |||
| prereq_roles: | |||
There was a problem hiding this comment.
We should just call them "roles" or "pulp_roles". Since they aren't prerequisites, they are the meat of the installer.
molecule/dynamic/playbook.yml
Outdated
|
|
||
| - name: Dynamically include prereq roles | ||
| include_role: | ||
| public: yes |
There was a problem hiding this comment.
Oh, I didn't know about this option. It is newish (Ansilbe 2.7).
This 1 option may solve the entire problem that led us to want to test include_role to begin with.
Did you try leaving it out? According to 1the Katello team (and 1 or 2 other users), they had to workaround an issue by setting a variable globally ("vars:" in playbook.yml.) I forget if that issue is still present.
There was a problem hiding this comment.
without it, it didn't work, because one role was depending on the vars of other roles. So it was breaking due to "missing vars".
There was a problem hiding this comment.
Let's figure out if our users are OK with "public" or not for the long-term. I reached out to @jlsherrill .
In the meantime, let's figure out that variable and try to address it as a separate PR that this is rebased on. (Or as a separate commit on this PR.) I think one technique we've used in the past is to do "loose coupling", putting the defaults in both roles that need the variable (even though the variable primarily pertains to 1 role.)
There was a problem hiding this comment.
done! Added a commit for it here!
There was a problem hiding this comment.
It's awesome that you tracked down all these missing variables. I didn't realize there were so many.
There was a problem hiding this comment.
So with all the variables added to the defaults, do we still need "public"?
There was a problem hiding this comment.
no, we don't need public, but I wasn't sure if I had to remove it, wdyt? Remove it or not?
There was a problem hiding this comment.
I decided to remove it, I think it is better for CI to see if it breaks when missing some variable
roles/pulp-content/defaults/main.yml
Outdated
| pulp_content_bind: "127.0.0.1:24816" | ||
| pulp_config_dir: "/etc/pulp" | ||
| pulp_settings_file: "{{ pulp_config_dir }}/settings.py" | ||
| pulp_install_dir: "/usr/local/lib/pulp" | ||
| pulp_user: pulp |
There was a problem hiding this comment.
The role's README.md needs an update about the loose coupling. See pulp-content/README.md's "Shared variables"
| pulp_settings_file: "{{ pulp_config_dir }}/settings.py" | ||
| pulp_default_admin_password: "" | ||
| # Users should not set this variable, instead using `pulp_settings.databases` | ||
| pulp_settings_db_defaults: |
There was a problem hiding this comment.
The role's README.md needs an update about the loose coupling between pulp and pulp-database. See pulp-content/README.md's "Shared variables"
There was a problem hiding this comment.
it is already there:
https://github.com/pulp/ansible-pulp/blob/master/roles/pulp/README.md
There was a problem hiding this comment.
I dislike the wording, but I suppose it is good enough.
tox.ini
Outdated
| @@ -42,3 +42,12 @@ commands = | |||
| {env:TRAVIS_WAIT:} molecule idempotence -s source | |||
| molecule side-effect -s source | |||
| molecule verify -s source | |||
| molecule lint -s dynamic | |||
There was a problem hiding this comment.
I recently fixed a bug in tox.ini; the "destroy" stage needs to be added for the "source" scenario before we run the "dynamic" scenario.
In other words, only the last scenario should lack a "destroy" task. (It wastes time, and makes debugging more difficult.) (Maybe we should add a comment for this.)
|
FYI: I made a bunch of comments, then made another. It may show only 1 change requested, but there are multiple. |
| pulp_settings_file: "{{ pulp_config_dir }}/settings.py" | ||
| pulp_default_admin_password: "" | ||
| # Users should not set this variable, instead using `pulp_settings.databases` | ||
| pulp_settings_db_defaults: |
|
Also @fabricio-aguiar , the "Loose coupling pulp roles" commit is fairly substantial. I would recommend quickly creating a redmine ticket real quick so as to ensure that you get credit for making this improvement that addresses many users' use cases. |
| pulp_settings_file: "{{ pulp_config_dir }}/settings.py" | ||
| pulp_default_admin_password: "" | ||
| # Users should not set this variable, instead using `pulp_settings.databases` | ||
| pulp_settings_db_defaults: |
There was a problem hiding this comment.
I dislike the wording, but I suppose it is good enough.
https://pulp.plan.io/issues/5766
closes #5766