Skip to content

Rename labels from sigs.k8s.io to machine.openshift.io #161

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Feb 23, 2019
Merged

Rename labels from sigs.k8s.io to machine.openshift.io #161

merged 2 commits into from
Feb 23, 2019

Conversation

spangenberg
Copy link

/cc bison

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 18, 2019
@enxebre
Copy link
Member

enxebre commented Feb 18, 2019 •
edited
Loading

this will need a counter part PR on the installer for the machine manifests. Can you add openshift/installer#1263 to the description for reference

@spangenberg
Copy link
Author

this will need a counter part PR on the installer for the machine manifests

openshift/installer#1263

enxebre
enxebre reviewed Feb 18, 2019
View reviewed changes
examples/machine-set.yaml
spec:
replicas: 2
selector:
matchLabels:
sigs.k8s.io/cluster-api-machineset: test-master
sigs.k8s.io/cluster-api-cluster: tb-asg-35
machine.openshift.io/cluster-api-machineset: test-master
Copy link
Member

Choose a reason for hiding this comment

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

for consistency, should this be machine-api-machineset and same for all cluster-api references?

Copy link
Author

Choose a reason for hiding this comment

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

Don't have any opinion on that, maybe some others can chime in here.

Copy link
Member

@ingvagabund ingvagabund Feb 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

itdoesnotmatter

because it's only for convenience. Though, if it does, go with machine.openshift.io/machineset-id

IMHO, cluster can stay as it is.

Copy link

Choose a reason for hiding this comment

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

I agree with @ingvagabund I guess. It doesn't really matter. There's an argument to be made for keeping it exactly like upstream except for the domain prefix though. If I were picking anything I wanted, I'd call it machine.openshift.io/machineset I think. I don't know... maybe this isn't helpful at all.

enxebre
enxebre reviewed Feb 18, 2019
View reviewed changes
test/e2e/provider_expectations.go Outdated
@@ -140,7 +140,7 @@ func (tc *testConfig) ExpectNewNodeWhenDeletingMachine() error {
var triagedWorkerNode corev1.Node
MachineLoop:
for _, m := range machineList.Items {
if m.Labels["sigs.k8s.io/cluster-api-machine-role"] == "worker" {
if m.Labels["machine.openshift.io/cluster-api-machine-role"] == "worker" {
Copy link
Member

@enxebre enxebre Feb 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

this will never pass the tests unless you make it backward compatible with the installer labels

enxebre
enxebre reviewed Feb 18, 2019
View reviewed changes
test/e2e/provider_expectations.go Outdated
@@ -217,7 +217,7 @@ func (tc *testConfig) ExpectNodeToBeDrainedBeforeDeletingMachine() error {
}
for _, machineItem := range machineList.Items {
// empty or non-worker role skipped
if machineItem.Labels["sigs.k8s.io/cluster-api-machine-role"] == "worker" {
if machineItem.Labels["machine.openshift.io/cluster-api-machine-role"] == "worker" {
Copy link
Member

Choose a reason for hiding this comment

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

this will never pass the tests unless you make it backward compatible with the installer labels

@spangenberg spangenberg mentioned this pull request Feb 18, 2019
Merged
bison
bison approved these changes Feb 18, 2019
View reviewed changes
Copy link

@bison bison left a comment

Choose a reason for hiding this comment

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

/lgtm

test/e2e/provider_expectations.go
@@ -140,7 +140,7 @@ func (tc *testConfig) ExpectNewNodeWhenDeletingMachine() error {
var triagedWorkerNode corev1.Node
MachineLoop:
for _, m := range machineList.Items {
if m.Labels["sigs.k8s.io/cluster-api-machine-role"] == "worker" {
if m.Labels["machine.openshift.io/cluster-api-machine-role"] == "worker" || m.Labels["sigs.k8s.io/cluster-api-machine-role"] == "worker" {
Copy link

Choose a reason for hiding this comment

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

I won't block this PR on this, but we should really be using the already defined constants here.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 18, 2019
@spangenberg
Copy link
Author

/retest

enxebre
enxebre reviewed Feb 19, 2019
View reviewed changes
pkg/apis/awsproviderconfig/v1beta1/awsmachineproviderconfig_types.go
ClusterIDLabel = "sigs.k8s.io/cluster-api-cluster"
MachineRoleLabel = "sigs.k8s.io/cluster-api-machine-role"
MachineTypeLabel = "sigs.k8s.io/cluster-api-machine-type"
ClusterIDLabel = "machine.openshift.io/cluster-api-cluster"
Copy link
Member

@enxebre enxebre Feb 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@spangenberg this will never pass the test as it would break the installer unless you make it backward compatible since ClusterIDLabel it's used by getClusterID which is used to get aws instances

@enxebre
Copy link
Member

enxebre commented Feb 19, 2019 •
edited
Loading

@spangenberg Please check the CI logs before rerunning tests. Nodes are not coming up because of #161 (comment)

I0219 16:28:21.926343       1 controller.go:114] Running reconcile Machine for ci-op-n2822mps-8b330-master-0
I0219 16:28:21.926481       1 actuator.go:411] checking if machine exists
E0219 16:28:21.927013       1 actuator.go:415] error getting running instances: unable to get cluster ID for machine: "ci-op-n2822mps-8b330-master-0"
E0219 16:28:21.927035       1 controller.go:169] Error checking existence of machine instance for machine object ci-op-n2822mps-8b330-master-0; unable to get cluster ID for machine: "ci-op-n2822mps-8b330-master-0"

Once you fix and merge this PR. It will unblock integration tests here https://github.com/openshift/machine-api-operator/pull/213/files#diff-17c17f0cec2da748cc69098a4166b579

Then we need to make sure to sync openshift/origin#22090 and we can merge openshift/installer#1263 and eventually remove the deprecated labels from the other repos

This was referenced Feb 19, 2019
Merged
Merged
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2019
@ingvagabund
Copy link
Member

/retest

2 similar comments
@spangenberg
Copy link
Author

/retest

@spangenberg
Copy link
Author

/retest

@ingvagabund
Copy link
Member

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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

The pull request process is described here

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 Feb 22, 2019
@spangenberg
Copy link
Author

/test e2e-aws

@openshift-merge-robot openshift-merge-robot merged commit 99939da into openshift:master Feb 23, 2019
michaelgugino pushed a commit to mgugino-upstream-stage/cluster-api-provider-aws that referenced this pull request Feb 12, 2020
@randomvariable @k8s-ci-robot
Initial user facing documentation (openshift#161)
6552b10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enxebre enxebre enxebre left review comments

@ingvagabund ingvagabund ingvagabund left review comments

@bison bison bison approved these changes

Assignees

@bison bison

@ingvagabund ingvagabund

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@spangenberg @enxebre @ingvagabund @openshift-ci-robot @bison @openshift-merge-robot