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

control-service: set registry name correctly. #1331

Merged
merged 11 commits into from
Dec 2, 2022

Conversation

murphp15
Copy link
Collaborator

@murphp15 murphp15 commented Nov 15, 2022

Why

The format of the dockerconfigjson is wrong.
Previously the auth key would look like registry/repo e.g registry.gitlab.com/project.taurus/data-jobs-stg.
With all the repositories I tried to authenticate against it should only be the registry name e.g registry.gitlab.com/project.taurus.
I reached out the @mivanov1988 privately in slack to check with him that this new way would also work with AWS ECR.

What

This fixes it

How has this been tested

Printed out the template and looks good.

Signed-off-by: murphp15 [email protected]

@murphp15
Copy link
Collaborator Author

@mivanov1988 Can you please look at this PR.
For me this change allows it to pull from the private jfrog repos.
However is there some repos you were working with that I might need to test this against to make sure I'm not breaking anything.

I explain the details on the difference in this comment: https://github.com/vmware/versatile-data-kit/pull/694/files#r1023916365

@mivanov1988
Copy link
Collaborator

Have you tested it against AWS ECR?

@antoniivanov
Copy link
Collaborator

antoniivanov commented Nov 24, 2022

Please prefix commit and PR title with "control-service" (as it's named in the projects directory) and not "vdk-control-service" .

When I generate release notes - there are both vdk-control-service and control-service so I have to go and rename them to be the same.

Copy link
Collaborator

@antoniivanov antoniivanov left a comment

Choose a reason for hiding this comment

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

LGTM

@antoniivanov
Copy link
Collaborator

Have you tested it against AWS ECR?

I think this is fixing a bug - the macro was not working because erroneously it was picking non existing key (repository) from values.yaml (probably we've renamed it in values.yaml and forgot to update the macro).

@murphp15 Please describe slightly more detailed the problem you are solving in each change/PR. It's not good to make reviewers guess . See https://github.com/vmware/versatile-data-kit/wiki/How-to-prepare-a-new-PR#write-a-good-commit-message

@murphp15 murphp15 changed the title vdk-control-service: set registry name correctly. control-service: set registry name correctly. Nov 24, 2022
@murphp15
Copy link
Collaborator Author

@tozka @mivanov1988 how do I test against aws ECR?

@tozka I have added details to the description.

@antoniivanov
Copy link
Collaborator

antoniivanov commented Nov 24, 2022

@tozka @mivanov1988 how do I test against aws ECR?

If I understand correctly difference between two is

  • the new way the login is against '0123123721.dkr.ecr.us-west-2.amazonaws.com' (the registry)

  • before it was against '0123123721.dkr.ecr.us-west-2.amazonaws.com/repo-vdk-name' (the repository)

and both work . For example using

aws ecr get-login-password --region us-west-2 | docker login --username AWS --password-stdin 0123123721.dkr.ecr.us-west-2.amazonaws.com

one can login. If this works likely the other logins would work as well.

To test it you need AWS account and ECR registry created. I have those - and I executed above command and it works.

@mivanov1988
Copy link
Collaborator

LGTM

@murphp15
Copy link
Collaborator Author

@mivanov1988 @tozka
Given the test Antoni has done do you think it is safe to merge and assume it works with ECR otherwise I need an aws account created.
How would I go about getting that?

@antoniivanov
Copy link
Collaborator

Given the test Antoni has done do you think it is safe to merge and assume it works with ECR otherwise I need an aws account created.

I don't think we need special tests for AWS really. The way you login and pull or push images from any registry is the same.

There are some differences in how repository is created across different registries but this PR doesn't really do anything related to repository creation.

But I did tested it just in case using ECR (I've been wrong before). And that's enough. Let's merge it.

@murphp15 murphp15 enabled auto-merge (squash) November 29, 2022 14:53
@murphp15 murphp15 merged commit 2a8ce64 into main Dec 2, 2022
@murphp15 murphp15 deleted the person/murphp15/correct_format_for_secret branch December 2, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants