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

Optional use of listcredentials for uploading CNF images to Artifact Store #72

Merged

Conversation

sunnycarter
Copy link
Collaborator

@sunnycarter sunnycarter commented Aug 30, 2023

Work item

This changes the way image and helm chart upload is done for CNF. The default behaviour is the same as before, i.e. CLI user context permissions are used for helm push and image copy (import). This will fail if the user does not have subscription-wide permissions (documented in params.py and various docstrings).

It adds two optional features:

  1. source_local_docker_image specified in the config file. Only works for a single image. Means you don't need a remote ACR if you're just doing a quickstart or noddy CNF
  2. --no-subscription-permissions CLI flag. Changes behaviour to use manifest credentials for helm push and docker image copy (now via docker pull, docker tag and docker push). Requires docker to be installed. Slower but doesn't require subscription-wide permissions in order to access Artifact Store.

Additional changes:

  • Using az acr import instead of python ContainerRegistryManagementClient, because we shell out all the time anyway, and that client was a bit of a pain to use, and required knowledge of resource group names. Without it we can just ask the user to provide the ACR login server name.
  • Fixes to various exceptions to use ones that don't show the whole stack trace to screen
  • Small tidy up of artifact_manifest vs artifact. We don't use the concept of an artifact client for helm push or image upload (we didn't before). I haven't fixed this but I've had to add manifest credentials to artifacts so they are available.
  • We can't check that the source ACR exists before we start now, as we don't know the RG. A small downside but we will get a decent error I think.

Testing:

  • Single helm chart and image, source image, source ACR import, source ACR docker push pull
  • Multiple helm charts and images, same
  • Misspelled source_registry gives decent output
  • Bad permissions (had to fake as don't have the right subscription access)

@sunnycarter sunnycarter changed the title Use listcredentials for uploading CNF images to Artifact Store Optional use of listcredentials for uploading CNF images to Artifact Store Sep 1, 2023
Copy link
Collaborator

@jamiedparsons jamiedparsons left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for improving a whole bunch of output text - that sort of thing makes a real difference.

Do you think that there is any risk of regressing Windows support here? Worth a quick test?

@jamiedparsons jamiedparsons merged commit 1e5fd22 into add-aosm-extension Sep 5, 2023
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.

3 participants