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

registry: Remove dependency on Docker binary for login/logout #805

Merged
merged 4 commits into from May 20, 2020

Conversation

andrewsomething
Copy link
Member

This removed the runtime dependency on the Docker binary for the registry login/logout commands. It also adds integration tests for them.

This does not resolve #709 on its own, but it should provide a path forward for getting the correct permissions approved by Canonical.

@guardrails

This comment has been minimized.

@guardrails

This comment has been minimized.

@guardrails

This comment has been minimized.

@guardrails

This comment has been minimized.

Copy link
Contributor

@Verolop Verolop left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +255 to +256
dockerCreds := cf.GetCredentialsStore(authconfig.ServerAddress)
err = dockerCreds.Store(authconfig)
Copy link
Member

Choose a reason for hiding this comment

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

I think these two operations call cgo. I'm going to pull down your branch right now to check, but if so, it might make our release process more complicated since we may not be able to use Go's built-in cross compilation as part of goreleaser.

Copy link
Member

Choose a reason for hiding this comment

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

Okay – they do call cgo, but it seems like it does not affect our ability to use Go's usual cross compilation tools, so I think this is fine.

Copy link
Member

@bentranter bentranter 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 to me!

Copy link
Contributor

@kamaln7 kamaln7 left a comment

Choose a reason for hiding this comment

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

Nice! LGTM 💯

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.

permissions error with doctl snap accessing docker
4 participants