Skip to content

fix: use encodeURIComponent for the repository input #73

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

Merged
merged 2 commits into from
Jul 12, 2024

Conversation

bkinzle
Copy link
Collaborator

@bkinzle bkinzle commented Jul 12, 2024

  • the artifact registry's client will throw an error if a slash is in the repository's string value, uri-encoding will fix this.

* the artifact registry's client will throw an error if a slash is in the repository's string value, uri-encoding will fix this.
@bkinzle bkinzle self-assigned this Jul 12, 2024
@bkinzle bkinzle requested a review from glasser July 12, 2024 16:26
src/index.ts Outdated
@@ -125,9 +125,10 @@ export async function main(): Promise<void> {

let dockerRegistryClient: DockerRegistryClient | null = null;
let finalizeDockerRegistryClient: (() => Promise<void>) | null = null;
const artifactRegistryRepository =
const artifactRegistryRepository = encodeURIComponent(
Copy link
Member

Choose a reason for hiding this comment

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

Is this the right spot? I think this is where the action reads the top level repository from the github workflows config, not the individual docker image from the application-values.yaml file. (It's incredibly confusing that AR and Docker use "repository" to mean different things.)

Copy link
Collaborator Author

@bkinzle bkinzle Jul 12, 2024

Choose a reason for hiding this comment

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

were you thinking it should go somewhere within the ArtifactRegistryDockerRegistryClient class itself?

@bkinzle bkinzle merged commit 9e8ccde into main Jul 12, 2024
5 checks passed
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