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

Add arbitrary labels to docker images #11209

Merged
merged 2 commits into from
Oct 13, 2016

Conversation

mmilata
Copy link
Contributor

@mmilata mmilata commented Oct 4, 2016

Trello card: https://trello.com/c/HB9IwGj5/925-5-add-arbitrary-labels-to-docker-images
Documentation: openshift/openshift-docs#3015

Missing:

@@ -622,6 +622,19 @@ type BuildOutput struct {
// up the authentication for executing the Docker push to authentication
// enabled Docker Registry (or Docker Hub).
PushSecret *kapi.LocalObjectReference `json:"pushSecret,omitempty" protobuf:"bytes,2,opt,name=pushSecret"`

// labels define a list of labels that are applied to the resulting image. If there are
// multiple labels with the same name then the last one in the list is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

validation should complain about multiple labels with the same name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was that the BuildDefaults admission plugin will prepend labels to the list making them effectively defaults and BuildOverrides will append them making them mandatory. Not really a problem though, this logic can be extended to remove the duplicate entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed now.


// labels define a list of labels that are applied to the resulting image. If there are
// multiple labels with the same name then the last one in the list is used.
Labels []ImageLabel `json:"labels,omitempty" protobuf:"bytes,3,rep,name=labels"`
Copy link
Contributor

Choose a reason for hiding this comment

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

it's confusing that these will result in docker labels, not API labels on the object that the to reference points to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I rename the field to imageLabels or dockerLabels?

Copy link
Contributor

@liggitt liggitt Oct 4, 2016

Choose a reason for hiding this comment

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

I'd vote for imageLabels, curious what others think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the field to imageLabels.

@mmilata mmilata force-pushed the arbitrary-image-labels branch 2 times, most recently from 577ed67 to 2e6f31e Compare October 5, 2016 19:28
@mmilata
Copy link
Contributor Author

mmilata commented Oct 5, 2016

updated:

  • implemented admission plugins
  • addressed issues found in review

@mmilata mmilata force-pushed the arbitrary-image-labels branch from 2e6f31e to 5f7ed01 Compare October 6, 2016 13:58
@mmilata
Copy link
Contributor Author

mmilata commented Oct 6, 2016

Added simple extended test.

@mmilata
Copy link
Contributor Author

mmilata commented Oct 7, 2016

Documentation: openshift/openshift-docs#3015

@mmilata mmilata force-pushed the arbitrary-image-labels branch from 5f7ed01 to aff38d0 Compare October 7, 2016 14:49
@mmilata mmilata changed the title [WIP] Add arbitrary docker labels to docker images Add arbitrary labels to docker images Oct 7, 2016
@mmilata
Copy link
Contributor Author

mmilata commented Oct 7, 2016

@bparees PTAL

@bparees
Copy link
Contributor

bparees commented Oct 10, 2016

[test]

@bparees
Copy link
Contributor

bparees commented Oct 10, 2016

[testextended][extended:core(builds)]

@bparees
Copy link
Contributor

bparees commented Oct 10, 2016

@mmilata please squash the 2 non-bump commits
@openshift/api-review please review/sign off.

@smarterclayton
Copy link
Contributor

API approved

@bparees
Copy link
Contributor

bparees commented Oct 10, 2016

flake #11240
[merge]

@mmilata mmilata force-pushed the arbitrary-image-labels branch from aff38d0 to d2c807f Compare October 10, 2016 14:23
@mmilata
Copy link
Contributor Author

mmilata commented Oct 10, 2016

@bparees, squashed & rebased on current master

@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to d2c807f

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to d2c807f

@bparees
Copy link
Contributor

bparees commented Oct 10, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to d2c807f

@mmilata
Copy link
Contributor Author

mmilata commented Oct 10, 2016

Both failures seem to be caused by AWS capacity problem:

InsufficientInstanceCapacity => We currently do not have sufficient m4.xlarge capacity in the Availability Zone you requested (us-east-1d). Our system will be working on provisioning additional capacity. You can currently get m4.xlarge capacity by not specifying an Availability Zone in your request or choosing us-east-1c, us-east-1b.

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/565/) (Extended Tests: core(builds))

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9844/)

@mmilata
Copy link
Contributor Author

mmilata commented Oct 11, 2016

looks like flake #11024

@mfojtik
Copy link
Contributor

mfojtik commented Oct 11, 2016

[merge]

@mmilata
Copy link
Contributor Author

mmilata commented Oct 12, 2016

@mfojtik, why is the bot ignoring this?:)

@mfojtik
Copy link
Contributor

mfojtik commented Oct 12, 2016

@mmilata it is not, see: #11209 (comment)

@openshift-bot
Copy link
Contributor

openshift-bot commented Oct 13, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9994/) (Base Commit: c728fe9) (Image: devenv-rhel7_5169)

@openshift-bot openshift-bot merged commit bc0a4dc into openshift:master Oct 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants