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

API changes #64

Merged
merged 4 commits into from
Aug 31, 2016
Merged

API changes #64

merged 4 commits into from
Aug 31, 2016

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Aug 31, 2016

… immediately needed for #52 , but also #56 was done expecting these API changes to occur.

Primarily extends usage of types.SystemContext to creation of transport objects. See individual commits for detailed comments. Should not change behavior.

for _, c := range []struct {
ctx *types.SystemContext
expected string
}{
Copy link
Member

Choose a reason for hiding this comment

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

can we test this out with $HOME?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Test what? defaultPolicyPath does not use the home directory. (re: the other conversation about environment variables, that happens for ATOMIC_CONF in docker/lookaside.go:atomicConfPath, but not in this PR).

Do you mean testing that an explicit SignaturePolicyPath is not expanded if it contains $HOME?

Copy link
Member

@runcom runcom Aug 31, 2016

Choose a reason for hiding this comment

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

Do you mean testing that an explicit SignaturePolicyPath is not expanded if it contains $HOME?

yes this

@runcom
Copy link
Member

runcom commented Aug 31, 2016

lgtm

just a (silly) question (feel free to skip)

Approved with PullApprove

},
// No environment expansion happens in the overridden paths
// Path overridden
{&types.SystemContext{SignaturePolicyPath: variableReference}, variableReference},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like this?

Copy link
Member

Choose a reason for hiding this comment

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

yep

mtrmac added 4 commits August 31, 2016 21:13
As we add more config files, having a single knob to override them all
may simplify usage.

Note that this is not a chroot; it will override e.g. hard-coded /etc,
but $HOME is unaffected.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... instead of Docker-specific certPath and tlsVerify.

Also invert the sense of tlsVerify to make the default secure.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…ation

This allows the selection to be consistent across GetManifest and
GetSignatures (which will be needed by Docker lookaside).

The API change causes lots of churn, but ultimately it just moves the
real origin of the value from image.FromSource() to transport.NewImageSource(),
both of which are static for the life of the ImageSource.

Does not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
For lookaside signature store, and separating the read and write URLs,
we need to set up read-only and read-write states differently; having
read-write “delete” in dockerImageSource is incovenient.

In tue future, ImageSource.Delete will be a really poor fit for
docker-daemon:, where initializing the ImageSource causes the tarball
to be copied from the daemon.  We could instead implement the
docker-daemon source so that it only copies the tarball on demand, but
not sharing the object is much simpler.

This leaves the Docker implementation in docker_image_src.go to make
reviewing easier.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Aug 31, 2016

👍

Approved with PullApprove

@mtrmac mtrmac merged commit fa5d1ab into containers:master Aug 31, 2016
@mtrmac mtrmac deleted the api-changes branch August 31, 2016 19:23
mtrmac added a commit to containers/skopeo that referenced this pull request Aug 31, 2016
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