Skip to content

BUG 1702050: Add aws role labels back (revert #1474)#1686

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
enxebre:add-label
May 1, 2019
Merged

BUG 1702050: Add aws role labels back (revert #1474)#1686
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
enxebre:add-label

Conversation

@enxebre
Copy link
Copy Markdown
Member

@enxebre enxebre commented Apr 29, 2019 •
edited
Loading

We dropped aws role labels in #1474 to reduce the surface to what's actually needed by our core controllers. However having this lables provides better UX and searchability for end users and consumers like machineAutoscaler resource so we are putting them back

@enxebre
Add aws role labels back
0fe8619 
We dropped aws role labels in openshift#1474 to reducethe surface to what's actually needed by our core controllers. However having this lables provides better UX and searchability for end users and consumers like machineAutoscaler resource so we are putting them back
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 29, 2019
@spangenberg
Copy link
Copy Markdown
Contributor

spangenberg commented Apr 29, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2019
@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Apr 29, 2019

/retest

@sdodson
Copy link
Copy Markdown
Member

sdodson commented Apr 29, 2019

/approve
Read through the referenced PR and the motivation to remove and subsequently add them back make sense.

@wking
Copy link
Copy Markdown
Member

wking commented Apr 29, 2019

Are there docs for machine.openshift.io/* labels we can link? It would be nice to be able to distinguish between "machine API needs this" and "machine API doesn't care, but thinks you might find this helpful".

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Apr 29, 2019

@wking we don't have that documented yet, we'll have something soon

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Apr 29, 2019 •
edited
Loading

@wking FYI created openshift/machine-api-operator#306

@chancez
Copy link
Copy Markdown
Contributor

chancez commented Apr 29, 2019

Are these labels actually specific to the cloud? They seem fairly agnostic. Is there a standard convention, similar to node labels like OS/Role/Failure-Domain?

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Apr 30, 2019

/test e2e-aws-upgrade

@enxebre
Copy link
Copy Markdown
Member Author

enxebre commented Apr 30, 2019

@chancez is up to the consumer to set this labels today. We might consider some standard for pre populating from the controller side in the future

@chancez
Copy link
Copy Markdown
Contributor

chancez commented Apr 30, 2019 •
edited
Loading

@enxebre So in practice they're arbitrary right? So they're not at all AWS specific. Perhaps in that case you guys could just re-use the labels used for nodes to indicate the role? Would make it somewhat simpler to document and explain the labels.

Otherwise I guess maybe adjusting the PR title/commit description to indicate these aren't actually AWS specific labels, but rather just arbitrary labels set by the installer when using the AWS cloud provider.

@frobware
Copy link
Copy Markdown

frobware commented May 1, 2019

/lgtm

@frobware
Copy link
Copy Markdown

frobware commented May 1, 2019

/assign @staebler

@staebler
Copy link
Copy Markdown
Contributor

staebler commented May 1, 2019

/approve

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented May 1, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre, frobware, sdodson, spangenberg, staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2019
@michaelgugino
Copy link
Copy Markdown
Contributor

michaelgugino commented May 1, 2019

/retest

@abhinavdahiya abhinavdahiya changed the title Add aws role labels back BUG 1702050: Add aws role labels back (revert #1474) May 1, 2019
@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented May 1, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit aa70e4c into openshift:master May 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patrickdillon patrickdillon Awaiting requested review from patrickdillon

@sdodson sdodson Awaiting requested review from sdodson

Assignees

@spangenberg spangenberg

@frobware frobware

@staebler staebler

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.