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

problem: pulp_installer doesn't document required variables #328

Merged
merged 1 commit into from Jun 22, 2020

Conversation

mikedep333
Copy link
Member

@mikedep333 mikedep333 commented Jun 10, 2020

correctly or provide clear errors if they are unset

Solution:

  1. Error early if they are null, unset an empty string, or
    an empty dict with a clearer error message.
  2. Remove pointless setting of some to empty values.
  3. Correct the documentation: They have no default values.

fixes: #6958

Test vars:

pulp_install_plugins: {}
  # galaxy-ng: {}
  # pulp-ansible: {}
  # pulp-certguard: {}
  # pulp-container: {}
  # pulp-cookbook: {}
  # pulp-deb: {}
  # pulp-file: {}
  # pulp-gem: {}
  # pulp-maven: {}
  # pulp-npm: {}
  # pulp-python: {}
  # pulp-rpm:
  #   prereq_role: "pulp.pulp_rpm_prerequisites" # RPM needs a prereq_role: https://galaxy.ansible.com/pulp/pulp_rpm_prerequisites
pulp_settings:
  secret_key:
  content_origin: ""

@mikedep333 mikedep333 force-pushed the required-vars branch 2 times, most recently from 2f16b98 to d9d03bb Compare June 10, 2020 20:07
@pulpbot
Copy link
Member

pulpbot commented Jun 10, 2020

Attached issue: https://pulp.plan.io/issues/6958

@mikedep333
Copy link
Member Author

I'm running into some weird syntax errors. I'll resume work on this after #326

@mikedep333
Copy link
Member Author

@dkliban I made a bunch of changes when I rebased and fixed this, so requesting re-review.

CHANGES/6958.bugfix Outdated Show resolved Hide resolved
roles/pulp/README.md Outdated Show resolved Hide resolved
@@ -5,7 +5,6 @@ pulp_config_dir: '/etc/pulp'
# This var intentionally not advertised to users, because there is no
# foreseeable need for them to change the filename, only pulp_config_dir.
pulp_settings_file: '{{ pulp_config_dir }}/settings.py'
pulp_default_admin_password: ''
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to add a .removal notice for these two vars now being required?

Copy link
Member Author

Choose a reason for hiding this comment

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

My test last week was that the empty string default would fail mid-play. So it's not really a removal of any functionality. I suppose that users could be comparing to the empty string in their own playbook or roles, but it seems very unlikely.

set it in your variable
(e.g. pulp_installer/playbooks/example-use/group_vars/all)
and run pulp_installer again.
See https://pulp-installer.readthedocs.io/en/latest/ or
Copy link
Member

Choose a reason for hiding this comment

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

I like how helpful this error message is. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, it is all outputted as one line though.

I found instructions for the debug module (rather than assert) module to do multiple lines via an array, but it just becomes an array outputted as JSON, still one line.

@mikedep333 mikedep333 force-pushed the required-vars branch 3 times, most recently from a557714 to bc3cc12 Compare June 18, 2020 19:28
correctly or provide clear errors if they are unset

Solution:
1. Error early if they are null, unset an empty string, or
an empty dict with a clearer error message.
2. Remove pointless setting of some to empty values.
3. Correct the documentation: They have no default values.

fixes: #6958

Test vars:
```
pulp_install_plugins: {}
  # galaxy-ng: {}
  # pulp-ansible: {}
  # pulp-certguard: {}
  # pulp-container: {}
  # pulp-cookbook: {}
  # pulp-deb: {}
  # pulp-file: {}
  # pulp-gem: {}
  # pulp-maven: {}
  # pulp-npm: {}
  # pulp-python: {}
  # pulp-rpm:
  #   prereq_role: "pulp.pulp_rpm_prerequisites" # RPM needs a prereq_role: https://galaxy.ansible.com/pulp/pulp_rpm_prerequisites
pulp_settings:
  secret_key:
  content_origin: ""
```
Copy link
Member

@bmbouter bmbouter left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @mikedep333 !

@mikedep333 mikedep333 merged commit 992ec83 into pulp:master Jun 22, 2020
@mikedep333 mikedep333 deleted the required-vars branch June 22, 2020 19:20
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.

None yet

4 participants