Skip to content
This repository was archived by the owner on Sep 9, 2020. It is now read-only.

Add docker image resolver to param set#645

Merged
bryanl merged 1 commit intoksonnet:masterfrom
bryanl:569-resolve-docker-images
Jun 27, 2018
Merged

Add docker image resolver to param set#645
bryanl merged 1 commit intoksonnet:masterfrom
bryanl:569-resolve-docker-images

Conversation

@bryanl
Copy link
Member

@bryanl bryanl commented Jun 24, 2018

Adds an image resolve to param set. eg:

ks param set deployment image foo/bar:latest --resolve-image

uses the docker registry to find the manifest reference for foo/bar:latest.
It then sets this value instead. Support is at the component and environment
level.

Fixes #569

Signed-off-by: bryanl bryanliles@gmail.com

@bryanl bryanl requested a review from a team June 24, 2018 21:50
@coveralls
Copy link

coveralls commented Jun 24, 2018

Pull Request Test Coverage Report for Build 1041

  • 72 of 105 (68.57%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 70.852%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/actions/param_set.go 22 28 78.57%
pkg/util/dockerregistry/registry.go 7 17 41.18%
pkg/util/dockerregistry/resolver.go 33 50 66.0%
Totals Coverage Status
Change from base Build 1040: -0.3%
Covered Lines: 10491
Relevant Lines: 14807

💛 - Coveralls

@bryanl bryanl force-pushed the 569-resolve-docker-images branch from 670bd2d to bd1e8e8 Compare June 25, 2018 01:54
Copy link
Collaborator

@underrun underrun left a comment

Choose a reason for hiding this comment

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

some of the stuff removed in this PR doesn't seem fully relevant to resolving images from relative labels/tags - why is it necessary to remove them in this PR?

OptionPath = "path"
// OptionQuery is query option.
OptionQuery = "query"
// OptionResolveImage is resolve image option. It used to resolve docker image references
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/It used/It is used/

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

)

const (
mimeTypeDockerManifest = "application/vnd.docker.distribution.manifest.v2+json"
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to use the OCI way of getting content addressable identifiers (with application/vnd.oci.descriptor.v1+json MIME type) or do we need to stay docker specific?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, in this case, application/vnd.docker.distribution.manifest.v2+json is the appropriate choice. With this MIME type, the server will set a header with the appropriate reference.

}

// Digester resolves digests for a docker image.
type Digester interface {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this name feels off to me ... how about Identifier or ContentAddressableIdentifier

Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed it to ResolverClient

@bryanl
Copy link
Member Author

bryanl commented Jun 27, 2018

@underrun the code removed was all dead. It needed to be removed.

@bryanl bryanl force-pushed the 569-resolve-docker-images branch from bd1e8e8 to 3f3e1fa Compare June 27, 2018 17:29
@bryanl
Copy link
Member Author

bryanl commented Jun 27, 2018

@underrun ping

Adds an image resolve to param set. eg:

`ks param set deployment image foo/bar:latest`

uses the docker registry to find the manifest reference for `foo/bar:latest`.
It then sets this value instead. Support is at the component and environment
level.

Fixes ksonnet#569

Signed-off-by: bryanl <bryanliles@gmail.com>
@bryanl bryanl force-pushed the 569-resolve-docker-images branch from 3f3e1fa to d3f430f Compare June 27, 2018 19:57
@bryanl bryanl merged commit 90f968a into ksonnet:master Jun 27, 2018
@bryanl bryanl deleted the 569-resolve-docker-images branch June 27, 2018 22:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Image resolver was removed

3 participants