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
Single container #210
Single container #210
Conversation
2adb4b9
to
7ffb9c4
Compare
| AWS_S3_ENDPOINT_URL = "http://minio:9000" | ||
| DEFAULT_FILE_STORAGE = "storages.backends.s3boto3.S3Boto3Storage" | ||
| AWS_DEFAULT_ACL = "@none None" | ||
| {% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include the pulp_settings from the template config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't as trivial as previously thought.
I added an ansible filter around pythons repr to make it work.
|
@mdellweg Per the meeting, give me a little more time to finish my review. |
210f72d
to
cd3079e
Compare
cd3079e
to
31495d4
Compare
| plugins: | ||
| - name: pulpcore | ||
| source: pulpcore{{ pulpcore_pip_version_specifier | default(omit, true) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: Later on, we're going to need special logic for pulpcore. I suppose we could always change this later:
https://pulp.plan.io/issues/5864
| plugins: | ||
| - name: pulpcore | ||
| source: pulpcore{{ pulpcore_pip_version_specifier | default(omit, true) }} | ||
| - name: {{ plugin_name }} | ||
| source: ./{{ plugin_name }} | ||
| {%- for item in additional_plugins %} | ||
| - name: {{ item.name }} | ||
| source: {{ item.name }}{{ item.pip_version_specifier | default(omit, true) }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we currently have 3 different data structures for our list of plugins to install across our installers. The addition of the 2nd & 3rd are my fault, but we should still try to consolidate them, even if certain args are ignored in different installers.
- pulp_installer
pulp_install_plugins - pulpcore.git/containers
plugins - template_config.yml
additional_plugins
That said, I do approve of your design though, because it is based on additional_plugins.
| {%- for repository in ["pulpcore", "pulp-smash", "pulp-openapi-generator"] + additional_plugins | map(attribute="name") | list %} | ||
| export {{ repository | upper | replace("-", "_") }}_PR_NUMBER=$(echo $COMMIT_MSG | grep -oP 'Required\ PR:\ https\:\/\/github\.com\/pulp\/{{ repository }}\/pull\/(\d+)' | awk -F'/' '{print $7}') | ||
| {%- endfor %} | ||
| else | ||
| export PULP_PR_NUMBER= | ||
| export PULP_SMASH_PR_NUMBER= | ||
| export PULP_ROLES_PR_NUMBER= | ||
| export PULP_BINDINGS_PR_NUMBER= | ||
| export PULP_OPERATOR_PR_NUMBER= | ||
| {%- for repository in ["pulpcore", "pulp-smash", "pulp-openapi-generator"] + additional_plugins | map(attribute="name") | list %} | ||
| export {{ repository | upper | replace("-", "_") }}_PR_NUMBER= | ||
| {%- endfor %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I was just brainstorming ways to make this more generic for https://pulp.plan.io/issues/6557
| # Install ansible with the boto3 tags to dict fix | ||
| pip install git+https://github.com/mdellweg/ansible.git@fix_boto3_tags_dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a PR against ansible?
I looked for one.
It also looks like ec2.py is no longer in the repo as of 2.10 devel (moved to a partner collection?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i did not find that file either in master. My guess is the humongous community-collection.
But actually i am unsure, whether the root problem here is with ansible, or minio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean.
I found the repo for Ansible 2.10 (git log -- ./deleted/file/path), the line is not fixed:
https://github.com/ansible-collections/amazon.aws/blob/master/plugins/module_utils/ec2.py#L458
It is published on Galaxy here, per Ansible's plan to separate partner modules from ansible core:
Could you please try reporting it to 1 or the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do. Thank you for finding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Please do 1 of the following so we don't end up with this forever:
- Put a comment above it with a link to the PR.
- Create a task in Pulp's redmine to make sure the PR gets accepted there (or in minio if they refuse.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment in place. I mentioned the move of the code to the collection, because this might be interesting anyway, when using the next version of ansible.
b86eec4
to
9dd39a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
9dd39a6
to
cb86c09
Compare
No description provided.