Skip to content
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

Support using multiple images sources in our CLI #142

Merged
merged 27 commits into from
Mar 26, 2024

Conversation

patrykkulik-microsoft
Copy link
Collaborator

@patrykkulik-microsoft patrykkulik-microsoft commented Feb 6, 2024

This PR is delivering the feature of allowing multiple image sources in our CLI and allow these sources to be registries other than Azure Container Registries (for example docker registries). Here is a link to the story: [Turtle 2Wk13 (Mar 10 - Mar 23) Taskboard - Boards (azure.com)](https://dev.azure.com/msazuredev/AzureForOperators/_sprints/taskboard/Turtle/AzureForOperators/Germanium/CY24Q1/2Wk/2Wk13%20(Mar%2010%20-%20Mar%2023)?workitem=1096825)

Changes

To achieve this, I have implemented the following:

  • I added a registry.py file which contains all logic related to registries
  • I have removed some of the registry related logic from the artifact.py file and moved it to the new registry classes
  • I have modified the input config file to allow for the new format of a list of registries. This means that the user will now specify the registries like:
"image_sources": [
    "Jl22FebMavPubJl22FebMavAcr7a5d480359.azurecr.io/mco",
    "ghcr.io/patrykkulik-microsoft/namespace/test/mco"
  ],
  • Given the changes above the flow will now be:
    • Specify all image sources in the config as a list of registries including namespace
    • create a registry handler in the onboarding_cnf_handler . This will create individual registry objects based on the input “image_sources” list
    • The registry handler will also check which images exists in the provided ACRs and make a dictionary of images and registries so that we can easily tell which image is in which registries
    • in registries other than ACRs, checking which images are in the registry is only possible by running a GET query outside of the docker interface. This is hard to do without asking the users for credentials. Because of this, I do not check for all images in the docker registries. Instead, if an image is not in an ACR, we search the docker registries for that specific image. This means that the interfaces between two registry classes are different. Is this ok? Doing the same for ACRs would make the process much slower.
    • In the helm chart processor we go through all images as before. But to get a registry to then pass to the artifact class, we have to run the find_registry_for_image on the registry_handler class.
    • if the image is not found in any registry, we raise a warning. This warning will happen at build stage. These artifacts are not going to be added to the list of artifacts so the publish step will still work fine if the user decides to ignore these warnings. Maybe we want to error out here?
    • The artifact class uploads images as before, although some code has moved

Other things:

  • I have removed the namespace from the images we obtain from helm template with the assumption that the string after the final slash is the actual image name while everything before that is just namespace? Is that true? When searching for images in the Registries, I add the namespace based on the user input in the input config.
  • some changes to how we create and read artifacts.json file
  • I added unit tests. I think these are good enough for now. Although a lot of my functions included running subprocesses and I wasn’t fully sure how to effectively test them. Would love some feedback here.
  • Nexus Vnfs also have interaction with container images. I had to make a small change in the vnf code to make sure that it uses the correct interface, but otherwise I have not made any changes. I don't think anything else is in the scope of this story. Let me know if you disagree.

Testing:

  • I have tested this with Mavenir's chart. I had some of the images that needed to be uploaded in an ACR and the rest in a Github registry (these were dummy images that just had the correct name for security reasons). I tested with a few different namespaces ways of adding the strings in the "image_sources". I am still yet to test with the quickstart nginx to see if that is fine
  • I have not yet done any Nexus Vnf tests. I need to check with Jordan about that.
  • I have a small worry that I might have broken something that is related to artifact classes so if you have any other test recommendations, I would love to hear them

src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
@@ -34,15 +34,10 @@ def write(self):
"""Write the definition element to disk."""
self.path.mkdir(exist_ok=True)
artifacts_list = []
# TODO: Handle converting path to string that doesn't couple this code to the artifact.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this all fixed? Amazing!!! 😃

Do you know how well it's been tested / needs to be tested?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this for the RemoteACRArtifact class which is the one that gets created for CNF helm images. I have not tested with other artifacts so would be great if someone tests it, but I don't think it should cause any problems given it works for mine just fine

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the next step is here. Do you think your live testing will cover this sufficiently?

src/aosm/HISTORY.rst Outdated Show resolved Hide resolved
src/aosm/HISTORY.rst Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
src/aosm/azext_aosm/common/registry.py Outdated Show resolved Hide resolved
@Cyclam
Copy link
Collaborator

Cyclam commented Mar 26, 2024

LGTM

@patrykkulik-microsoft patrykkulik-microsoft merged commit 0c90032 into main Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants