-
Notifications
You must be signed in to change notification settings - Fork 70
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
Use copy instead of template for disconnected registries.conf #808
Use copy instead of template for disconnected registries.conf #808
Conversation
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.
Did we not test it at all? That would have never worked.
I checked the container I built to test this in my lab, it used |
ansible.builtin.template: | ||
src: "{{ edpm_podman_registries_conf }}" | ||
ansible.builtin.copy: | ||
content: "{{ edpm_podman_registries_conf }}" |
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 LGTM as a fix.
I missed reviewing the original patch that added the variable. I think we need a way to distinguish between variables that are meant to be set by our code (openstack-operator in this case), and user variables. As it is now, a user has no way of knowing that they shouldn't use this variable themselves, and if they do, it might not even take effect as they intend.
Just looking at the variable, and the description in argument_specs.yml, as I user I would expect I could use this to define the registries.conf content. When in fact, it only works with disconnected, and could also be overwritten by openstack-operator.
I think we should file an issue to clean this sort of thing up across all the code, b/c I'm sure there are other examples.
0fffc77
to
f7f124d
Compare
Adoption job failed:
Seems unrelated:
|
recheck |
Same thing on recheck. There's something that needs fixing in the adoption job I guess:
|
|
f7f124d
to
fbd3941
Compare
This change switches the Ansible module from template to copy for writing the disconnected registries.conf file. Since trying to use `src` in the `template` module looks for a path and not content. Signed-off-by: Brendan Shephard <bshephar@redhat.com>
fbd3941
to
a4ac54c
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bshephar, rabi, slagle The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
06d9702
into
openstack-k8s-operators:main
/cherry-pick 18.0-fr1 |
@bshephar: new pull request created: #833 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This change switches the Ansible module from template to copy for writing the disconnected registries.conf file. Since trying to use
src
in thetemplate
module looks for a path and not content.Jira: https://issues.redhat.com/browse/OSPRH-11475