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

[FIX] Changing env to mountable path - PRIVATE_REGISTRY_CREDENTIAL_PATH #292

Merged
merged 7 commits into from
Apr 30, 2024

Conversation

harini-venkataraman
Copy link
Contributor

What

Initally the credentials for authenticating private images were directly set to the env variable.
This PR changes to a mountable path.
...

Why

This change helps automating On-prem deployents
...

How

Introducing PRIVATE_REGISTRY_CREDENTIAL_PATH for uploading the mount path.

...

Can this PR break any existing features. If yes please list of possible items. If no please exaplin why. (PS: Admins do not merge the PR without this section filled)

This PR will not break existing features.
It introduces an env, where the path of SA Key file is mounted.
If the file is not present, but the env is set - We do not raise exception rather catch and log.
This would prevent the constructor from failing in cases where the private tool is not used.

...

Database Migrations

Not applicable

...

Env Config

PRIVATE_REGISTRY_CREDENTIAL_PATH - Path where the credential file is mounted.

...

Relevant Docs

Not applicable

...

Checklist

I have read and understood the Contribution Guidelines.

@harini-venkataraman harini-venkataraman self-assigned this Apr 25, 2024
@harini-venkataraman harini-venkataraman changed the title Changing env to mountable path - PRIVATE_REGISTRY_CREDENTIAL_PATH [FIX] Changing env to mountable path - PRIVATE_REGISTRY_CREDENTIAL_PATH Apr 25, 2024
@harini-venkataraman harini-venkataraman added the bug Something isn't working label Apr 25, 2024
Copy link
Contributor

@gaya3-zipstack gaya3-zipstack left a comment

Choose a reason for hiding this comment

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

Also, do we need any docker-compose file changes for mounting the file?

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@harini-venkataraman
Copy link
Contributor Author

Also, do we need any docker-compose file changes for mounting the file?

@ritwik-g You have any comments to add here ?

Copy link
Contributor

@gaya3-zipstack gaya3-zipstack 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

@gaya3-zipstack
Copy link
Contributor

@harini-venkataraman @nehabagdia I have approved the PR. However I and Harini discussed a few points in propagating this error to the end user in a readable and meaningful way. @harini-venkataraman mentioned that she will address it once @muhammad-ali-e 's work gets merged.

@harini-venkataraman
Copy link
Contributor Author

@harini-venkataraman @nehabagdia I have approved the PR. However I and Harini discussed a few points in propagating this error to the end user in a readable and meaningful way. @harini-venkataraman mentioned that she will address it once @muhammad-ali-e 's work gets merged.

@gaya3-zipstack @nehabagdia Have raised a issue to track the same
https://zipstack.atlassian.net/browse/UN-1214

@nehabagdia nehabagdia merged commit 42d784d into main Apr 30, 2024
4 checks passed
@nehabagdia nehabagdia deleted the image-auth branch April 30, 2024 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants