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

pulp_rpm_prerequisites role into pulp_installer #321

Merged
merged 1 commit into from Jun 18, 2020
Merged

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Jun 5, 2020

@pulpbot
Copy link
Member

pulpbot commented Jun 5, 2020

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

molecule/release-dynamic/host_vars/debian-10 Outdated Show resolved Hide resolved
molecule/release-static/host_vars/debian-10 Outdated Show resolved Hide resolved
docs/quickstart.md Outdated Show resolved Hide resolved
molecule/release-upgrade/molecule.yml Show resolved Hide resolved
@fao89 fao89 force-pushed the 6799 branch 6 times, most recently from 92917e4 to 699a3da Compare June 16, 2020 21:18
Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

See existing thread about CI changes.

@@ -12,8 +12,7 @@ pulp_install_plugins:
# 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-rpm: {}
Copy link
Member

Choose a reason for hiding this comment

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

So this playbook is actually meant for users to run. It is also used in molecule CI. If we want molecule CI to use it, it should be copied to molecule/release-static/group_vars/all (and symlinked from other molecule tests) rather than symlinked into molecule folders.

Copy link
Member

Choose a reason for hiding this comment

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

I see you created the new copies of the vars files under molecule/, but now you need to restore these example-use/ vars files to their original values (except for removing prereq_role).

Comment on lines 27 to 28
pulp-rpm:
source_dir: "/var/lib/pulp/devel/pulp_rpm"
Copy link
Member

Choose a reason for hiding this comment

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

So this playbook is actually meant for developers to run. It is also used in molecule CI. If we want molecule CI to use it, it should be copied to molecule/source-static/group_vars/all (and symlinked from other molecule tests) rather than symlinked into molecule folders.

Copy link
Member

Choose a reason for hiding this comment

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

I see you created the new copies of the vars files under molecule/, but now you need to restore these example-use/ vars files to their original values (except for removing prereq_role).

Comment on lines 54 to 66
- name: Enable/Disable pulp-rpm tests
env:
DISABLE_PULP_RPM_TEST: ${{ secrets.DISABLE_PULP_RPM_TEST }}
run: |
if [[ "$DISABLE_PULP_RPM_TEST" == 'true' ]]; then
find ./molecule/*/group_vars/all -exec sh -c "yq d -i {} pulp_install_plugins.pulp-rpm" \;
find ./molecule/*/group_vars/all -exec sh -c "echo; echo {}; cat {}" \;
fi
shell: bash
Copy link
Member Author

@fao89 fao89 Jun 17, 2020

Choose a reason for hiding this comment

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

@pulp/ansible-installer I came up with this idea, we can disable pulp-rpm test by setting a secret for this repo, wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Like an environment variable that if present would cause it to skip?

Copy link
Member Author

Choose a reason for hiding this comment

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

exactly!

Copy link
Member

Choose a reason for hiding this comment

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

That sounds great! Let's document it in the docs somehow for developers please too.

Copy link
Member Author

Choose a reason for hiding this comment

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

so we can avoid:
#321 (comment)
without touching the code

Copy link
Member Author

Choose a reason for hiding this comment

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

@bmbouter only maintainers can change the repo environment variables, so I'm not sure where to document it

Copy link
Member

Choose a reason for hiding this comment

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

I think in the part of the docs that talk about tests. Say for example someone went to run the tests locally and they wanted to know how to set the environment var locally. I don't see that section in the docs so I propose make a new one.

Copy link
Member Author

@fao89 fao89 Jun 17, 2020

Choose a reason for hiding this comment

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

the environment variable trick is only used on the CI, I mean, it is checked at GHA workflow

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. I was imagining a skip-test on the test itself, but you're saying (I think) that this disables part of the matrix. If it's that second one then disregard my suggestions to document because it's only in the CI like I'm saying.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it is the second case. The matrix will install pulp-file and pulp-rpm regularly, but due release, we can remove pulp-rpm from the matrix (sometimes pulp-rpm is released couple days later than pulpcore/pulp-file which breaks the CI)

.github/workflows/pulp_ci.yaml Show resolved Hide resolved
molecule/release-upgrade/molecule.yml Show resolved Hide resolved
Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Almost there!

Comment on lines 27 to 28
pulp-rpm:
source_dir: "/var/lib/pulp/devel/pulp_rpm"
Copy link
Member

Choose a reason for hiding this comment

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

I see you created the new copies of the vars files under molecule/, but now you need to restore these example-use/ vars files to their original values (except for removing prereq_role).

@@ -12,8 +12,7 @@ pulp_install_plugins:
# 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-rpm: {}
Copy link
Member

Choose a reason for hiding this comment

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

I see you created the new copies of the vars files under molecule/, but now you need to restore these example-use/ vars files to their original values (except for removing prereq_role).

Copy link
Member

@mikedep333 mikedep333 left a comment

Choose a reason for hiding this comment

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

Well done! 🥇

@fao89 fao89 merged commit 66327ae into pulp:master Jun 18, 2020
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