-
Notifications
You must be signed in to change notification settings - Fork 141
[nat64_appliance] Download nat64 image #3545
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
base: main
Are you sure you want to change the base?
Conversation
59a9790 to
18458d8
Compare
hjensas
left a comment
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.
The molecule job passed, https://softwarefactory-project.io/zuul/t/rdoproject.org/build/637398ca48fc43fdbf147dd8ee4d19f9
LGTM. Thanks!
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.
LGTM
Edit: This has a blocker, so not approving.
| ansible.builtin.get_url: | ||
| url: "{{ cifmw_nat64_appliance_image_url }}" | ||
| dest: "{{ cifmw_nat64_appliance_workdir }}/nat64-appliance.qcow2" | ||
| checksum: "{{ cifmw_nat64_appliance_image_checksum | default(omit) }}" |
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 think this might make a problem as default value is "" so the omit is not raised and the checksum fails because is not an expected format.
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.
(non-blocking) suggestion: Could you add a new molecule scenariuo where that is cover? or a new molecule task that consumes getting an already created one?
Skip if not possible.
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.
Good catch, I think it will work as is - because the default for the get_url[1] module is "" for the checksum, and the condition in the module[2] is just if checksum ?
However, since there is a default setting it to "" in roles/nat64_appliance/defaults/main.yml I think the | default(omit) is "dead code"?
[1] https://docs.ansible.com/projects/ansible/latest/collections/ansible/builtin/get_url_module.html#parameter-checksum
[2] https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/get_url.py#L565
18458d8 to
c1fca20
Compare
c6c3979 to
11ceb03
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/c5cc6858dbe04c2e89d7c1764cee8166 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 43m 28s |
c344837 to
95f3db8
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/63e9f9813a844401bad32864652163e5 ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 55m 10s |
Introduce cifmw_nat64_appliance_image_url parameter to download pre-built NAT64 images, with optional checksum verification support. Signed-off-by: Miguel Angel Nieto Jimenez <[email protected]>
95f3db8 to
6961d06
Compare
Uh oh!
There was an error while loading. Please reload this page.