client: Allow setting image labels on Pull() by mlaventure · Pull Request #1551 · containerd/containerd

mlaventure · 2017-09-22T20:51:17Z

Signed-off-by: Kenfe-Mickael Laventure [email protected]

codecov-io · 2017-09-23T17:54:36Z

Codecov Report

Merging #1551 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1551   +/-   ##
=======================================
  Coverage   42.36%   42.36%           
=======================================
  Files          24       24           
  Lines        3368     3368           
=======================================
  Hits         1427     1427           
  Misses       1612     1612           
  Partials      329      329

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20c6211...ccc574f. Read the comment docs.

AkihiroSuda

LGTM, should we have the same stuff for Import() as well?

mlaventure · 2017-09-25T14:30:26Z

@AkihiroSuda probably, yes. Will add it, thanks.

crosbymichael · 2017-09-25T15:36:31Z

Couldn't this be part of the Opts?

I thought about it for Import.

Here, I found it weird to have it as part of a type called RemoteOpts, I guess I can rename it to PullOpts, lemme take a pass at it

@crosbymichael done. Lemme know if you prefer it that way, then I'll squash the commits.

dmcgowan · 2017-09-25T16:36:04Z

Could we just add Labels to RemoteContext? Many of the values on there are already pull specific, I don't think it makes sense to split the options just for the labels without splitting them all. We had a unified push/pull type to keep the options simpler and used the WithPull* nomenclature to differentiate. The fact that this necessitates WithPullRemoteOpt indicates to me that it should not exist.

estesp · 2017-09-25T17:31:53Z

several of your client.Pull changes from the first implementation now have a nil parameter that needs to be removed.

mlaventure · 2017-09-25T17:33:16Z

@estesp yes, just noticed, it's should be fixed 😇

mlaventure · 2017-09-26T17:58:15Z

rebased and squashed

estesp · 2017-09-26T18:38:40Z

is this a stray m or does it mean something? 😇

dang, yes it is. Probably a failed emacs command 😅

Thanks for noticing

stevvooe · 2017-09-27T22:02:21Z

Clarify that this only applies to the image record and not transitive resources.

stevvooe · 2017-09-27T22:04:16Z

LGTM

I'm assuming this is to avoid the transactional problems of labels after pull.

Signed-off-by: Kenfe-Mickael Laventure <[email protected]>

dmcgowan · 2017-09-28T17:17:18Z

LGTM

…9.0-rc.4 vendor: kubernetes 1.19.0 rc.4 and dependencies

mlaventure force-pushed the client-pull-set-labels branch 2 times, most recently from 4aa6580 to d4fd19e Compare September 23, 2017 17:47

AkihiroSuda approved these changes Sep 25, 2017

View reviewed changes

crosbymichael reviewed Sep 25, 2017

View reviewed changes

dmcgowan reviewed Sep 25, 2017

View reviewed changes

mlaventure force-pushed the client-pull-set-labels branch from 556324b to b8349d8 Compare September 25, 2017 17:22

mlaventure force-pushed the client-pull-set-labels branch from b8349d8 to 1b6e178 Compare September 25, 2017 17:33

mlaventure force-pushed the client-pull-set-labels branch from 1b6e178 to 83cad51 Compare September 26, 2017 17:58

estesp reviewed Sep 26, 2017

View reviewed changes

mlaventure force-pushed the client-pull-set-labels branch from 83cad51 to a0e1d21 Compare September 26, 2017 18:45

stevvooe reviewed Sep 27, 2017

View reviewed changes

Comment thread client.go Outdated

Copy link
Copy Markdown

Member

stevvooe Sep 27, 2017

Choose a reason for hiding this comment

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

Clarify that this only applies to the image record and not transitive resources.

client: Allow setting image labels on Pull() and Import()

df82159

Signed-off-by: Kenfe-Mickael Laventure <[email protected]>

mlaventure force-pushed the client-pull-set-labels branch from a0e1d21 to df82159 Compare September 27, 2017 22:05

dmcgowan merged commit 70b353d into containerd:master Sep 28, 2017

mauriciovasquezbernal pushed a commit to kinvolk/containerd that referenced this pull request Nov 13, 2020

Merge pull request containerd#1551 from thaJeztah/bump_kubernetes_1.1…

4e6644c

…9.0-rc.4 vendor: kubernetes 1.19.0 rc.4 and dependencies

Conversation

mlaventure commented Sep 22, 2017

Uh oh!

codecov-io commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

mlaventure commented Sep 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estesp commented Sep 25, 2017

Uh oh!

mlaventure commented Sep 25, 2017

Uh oh!

mlaventure commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Sep 27, 2017

Uh oh!

dmcgowan commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Sep 23, 2017 •

edited

Loading