-
Notifications
You must be signed in to change notification settings - Fork 57
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
Remove remaining container images from test roles #175
Remove remaining container images from test roles #175
Conversation
@@ -121,7 +121,6 @@ | |||
enabled: true | |||
template: | |||
ovnNorthd: | |||
containerImage: {{ container_registry|default("quay.io") }}/{{ container_namespace|default("podified-antelope-centos9") }}/openstack-ovn-northd:{{ container_tag | default("current-podified") }} |
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'm not sure if we can remove the ovnNorthd image, I don't see it in the ovn section of the container_overrides templates
data-plane-adoption/tests/roles/backend_services/templates/container_overrides.j2
Line 67 in 677d191
ovn: |
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'll add 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.
Done. I also fixed indentation where i noticed it being off in that file.
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.
right - I left these where they were and meant to cycle back and check if we really wanted/needed them hard-coded here.
thanks @jistr adding to the overrides should keep the period jobs doing the right thing, and otherwise it will default to current-podified
The two containerImage parameters removed here are the only ones remaining in the test roles. For normal usage, this is defaulted in the operators and we shouldn't need to set it. For periodic CI, these values are overriden with particular non-latest container URLs via the kustomization mechanism. So i think these shouldn't be necessary and can be removed too. Related: openstack-k8s-operators#110 openstack-k8s-operators#165
3d755be
to
b018949
Compare
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/280d598007af416086fefaea55bfcd22 ❌ data-plane-adoption-github-rdo-centos-9-crc-single-node RETRY_LIMIT in 20m 35s |
recheck |
recheck last run was
|
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 but lets wait for the test run to sanity check all the things are as expected here
@@ -40,7 +40,6 @@ | |||
enabled: true | |||
template: | |||
databaseInstance: openstack | |||
containerImage: {{ container_registry|default("quay.io") }}/{{ container_namespace|default("podified-antelope-centos9") }}/openstack-glance-api:{{ container_tag | default("current-podified") }} |
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.
ack we have
data-plane-adoption/tests/roles/backend_services/templates/container_overrides.j2
Lines 18 to 23 in 677d191
glance: | |
template: | |
glanceAPIInternal: | |
containerImage: {{ container_registry }}/{{ container_namespace }}/openstack-glance-api:{{ container_tag }} | |
glanceAPIExternal: | |
containerImage: {{ container_registry }}/{{ container_namespace }}/openstack-glance-api:{{ container_tag }} |
The two containerImage parameters removed here are the only ones remaining in the test roles. For normal usage, this is defaulted in the operators and we shouldn't need to set it.
For periodic CI, these values are overriden with particular non-latest container URLs via the kustomization mechanism.
So i think these shouldn't be necessary and can be removed too.
Related:
#110 #165