feat: simplify and generalise get_image() #405
Conversation
get_image function in image_utils.py had a hardcoded list of image names and code to modify these for use with a local target registry. It fails an assertion if an unexpected image name is passed in. This commit simplifies the code and has default functionality for any image not explicitly handled, so that old_registry/path/image_name is converted to new_registry/image_name which will work in almost all cases.
If the new optional argument --manifest is given to image_loader command, it reads the list of docker images to be uploaded into the local registry from the specified YAML file instead of using the built-in list of images. A suitable file can be generated by the playbook ansible-collection-kubernetes.image_manifest
magnum_cluster_api/image_utils.py
Outdated
| or new_image_name.startswith("registry.k8s.io/pause") | ||
| ): | ||
| return new_image_name.replace("registry.k8s.io", repository) | ||
| return name.replace("docker.io/calico/", f"{repository}/calico-") |
There was a problem hiding this comment.
@stuartgrace-bbc is there a typo here /calico- rather than /calico/ ?
Correction to names of calico and cilium images in local repo
| @@ -57,6 +58,10 @@ | |||
| required=True, | |||
| help="Target image repository", | |||
| ) | |||
| @click.option( | |||
| "--manifest", | |||
There was a problem hiding this comment.
--images-manifest would make it more explicit this is about images and not some other type of manifest (now or in the future). wdyt?
There was a problem hiding this comment.
@gtirloni we have updated this now to be --images-manifest
| + _get_calico_images() | ||
| + _get_cloud_provider_images() | ||
| + _get_infra_images() | ||
| ) |
There was a problem hiding this comment.
instead of keeping this code, could we supply a default manifest that's equivalent so people don't have to look into two different places (code and manifest) to figure this out?
There was a problem hiding this comment.
That would extend the scope of this PR as we would have to keep the default manifest in the source tree, manage its installation to a known location and then load it by default. I am conscious that we are making very little progress toward getting this merged so would like to keep the scope contained.
An extracted default manifest would be missing the images found through running kubeadm so I think that this might need more consideration, and should probably be a follow-up PR if anyone were wanting to do that work.
| @@ -56,31 +56,11 @@ def get_image(name: str, repository: str = None): | |||
| if not repository: | |||
| return name | |||
|
|
|||
| new_image_name = name | |||
| if name.startswith("docker.io/calico"): | |||
There was a problem hiding this comment.
are all of these hardcoded rules still needed here? I'm thinking they should error out if they are specified incorrectly.
There was a problem hiding this comment.
This PR removes all of the hardcoded rules that are not a simple replacement. The remaining ones do not conform to that pattern and as I understand it need to remain for compatibility with the registry/repository structure expected elsewhere in atmosphere(?). @mnaser may be able to give some more context on why things are like this.
Argument --manifest on command image_loader.py renamed to --images-manifest so it is clear what this contains.
|
I apologize this has taken a hot minute to review, I've been meaning to create unit tests to try and make sure we don't regress in those, since to be honest we don't really have a test that uses a |
|
@mnaser could you please take a quick look at https://github.com/bbc/magnum-cluster-api/commits/external-manifest/ This should be much lower risk and if it looks basically OK I can create another PR with that different approach. |
get_image function in image_utils.py had a hardcoded list of image names and code to modify these for use with a local target registry. It fails an assertion if an unexpected image name is passed in.
This commit simplifies the code and has default functionality for any image not explicitly handled, so that
old_registry/path/image_name
is converted to
new_registry/image_name
which will work in almost all cases.