Skip to content

Add Support for Google Cloud Artifact Registry #159

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jvanderen1
Copy link

@jvanderen1 jvanderen1 commented Jun 5, 2024

Issue: #158

Description

This adds the ability to specify a Google Cloud Artifact Registry repository and image with this action. Specifically:

@jvanderen1 jvanderen1 marked this pull request as draft June 5, 2024 05:09
@jvanderen1 jvanderen1 changed the title Add Support for Gcloud Artifact Registry Add Support for Google Cloud Artifact Registry Jun 5, 2024
@jvanderen1 jvanderen1 marked this pull request as ready for review June 5, 2024 06:22
@jvanderen1 jvanderen1 marked this pull request as draft June 5, 2024 06:35
@@ -300,6 +307,11 @@ init_variables() {

# split tags (to allow multiple comma-separated tags)
IFS=, read -ra INPUT_IMAGE_TAG <<< "$INPUT_IMAGE_TAG"

Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather move this condition as an elif insode _set_namespace (after the elif of _is_gcloud_container_registry.
Let me know if you want me to do it if it's not clear.

Thanks for the contribution!

Copy link
Author

Choose a reason for hiding this comment

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

I appreciate it man!

I was not sure if it would be confusing to call _set_namespace just to keep $NAMESPACE empty for _is_gcloud_artifact_registry. However, I can move this condition 👍🏻 .

Also, this PR is not quite ready yet. The error I had seen via #158 makes me believe I may had done something incorrect since this error is thrown in entrypoint.sh#L29-L32. I have not had a chance to test this PR yet, but I'll try to get to it at some point 🙂

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants