Skip to content

[ACR] Add OCI Artifact Source Context#14576

Merged
fengzhou-msft merged 5 commits into
Azure:devfrom
akashsinghal:akashsinghal/addOCIArtifactSupport
Aug 28, 2020
Merged

[ACR] Add OCI Artifact Source Context#14576
fengzhou-msft merged 5 commits into
Azure:devfrom
akashsinghal:akashsinghal/addOCIArtifactSupport

Conversation

@akashsinghal
Copy link
Copy Markdown
Contributor

@akashsinghal akashsinghal commented Jul 29, 2020

Description

Currently, ACR CLI does not support an OCI artifact to be passed in as the source context or source location for any of the commands. We are adding support for this new third type of remote context and need to add CLI support for contexts provided of form oci://myregistry.azurecr.io/artifact.

Testing Guide

az acr build -f Dockerfile -r myregistry oci://myregistry.azurecr.io/artifact:v1 This command will take the provided source context (oci://) and use the Dockerfile downloaded from this source context to build. (Need to change registry with own registry. Need to change artifact link with own artifact uploaded to registry)
az acr run -f acb.yaml -r myregistry oci://myregistry.azurecr.io/artifact:v2 This command will take the provided source context (oci://) and use the acb.yaml downloaded from this source context to run the task. (Need to change registry with own registry. Need to change artifact link with own artifact uploaded to registry)

This checklist is used to make sure that common guidelines for a pull request are followed.

@akashsinghal
Copy link
Copy Markdown
Contributor Author

@northtyphoon @shahzzzam

@northtyphoon
Copy link
Copy Markdown
Member

@shahzzzam @jaysterp @rosanch

commit_trigger_enabled = False
pull_request_trigger_enabled = False

if context_path.lower().startswith("oci://"):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@akashsinghal this will throw a null exception if the above case is met and we set context_path = None. Please add a null check here before calling .lower()

@yungezz
Copy link
Copy Markdown
Member

yungezz commented Aug 3, 2020

LGTM. Could you pls add some test?

Copy link
Copy Markdown
Contributor

@shahzzzam shahzzzam left a comment

Choose a reason for hiding this comment

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

lgtm

@fengzhou-msft
Copy link
Copy Markdown
Member

@akashsinghal can you fix the CI failure?

@Azure Azure deleted a comment from yonzhan Aug 4, 2020
@akashsinghal
Copy link
Copy Markdown
Contributor Author

@yungezz @fengzhou-msft Please do not merge this PR. These additions are reliant on new internal changes in ACR to be deployed to production and will be ready to merge after that has occurred. As for added tests, we will add a new test for this feature after global ACR deployment. Thanks! /cc: @northtyphoon @shahzzzam

@shahzzzam
Copy link
Copy Markdown
Contributor

This Looks good to me now. It's ok to merge this. Internal changes to ACR have now been deployed.

cc: @northtyphoon @yungezz @fengzhou-msft

@yungezz
Copy link
Copy Markdown
Member

yungezz commented Aug 27, 2020

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 2 pipeline(s).

@yungezz
Copy link
Copy Markdown
Member

yungezz commented Aug 27, 2020

hi @fengzhou-msft could you pls review and merge this PR? it's ready to merge now

@fengzhou-msft fengzhou-msft merged commit d121af6 into Azure:dev Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants