Skip to content

Get values for CRD's additionalPrinterColumns from annotations and labels instead of machine provider config#360

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
ingvagabund:get-machine-additional-columns-from-annotations-and-labels
Aug 8, 2019
Merged

Get values for CRD's additionalPrinterColumns from annotations and labels instead of machine provider config#360
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
ingvagabund:get-machine-additional-columns-from-annotations-and-labels

Conversation

@ingvagabund
Copy link
Copy Markdown
Member

@ingvagabund ingvagabund commented Jul 23, 2019 •
edited
Loading

Given we already support AWS, Azure and GCP instalations of OpenShift,
reading values from machine provider config is no longer an option.
Instead, let's read all needed values from machine annotations and labels.
Each actuator will be then responsible for making sure every machine
has all the annotations and labels that are needed to properly display
additional machine columns set.

@ingvagabund ingvagabund added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 23, 2019
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 23, 2019
Fedosin
Fedosin reviewed Jul 25, 2019
View reviewed changes
Comment thread install/0000_30_machine-api-operator_02_machine.crd.yaml Outdated
Comment thread install/0000_30_machine-api-operator_02_machine.crd.yaml Outdated
@Fedosin
Copy link
Copy Markdown
Contributor

Fedosin commented Jul 25, 2019

@ingvagabund could you also add OpenStack to the list of platforms in the PR description?

@ingvagabund ingvagabund force-pushed the get-machine-additional-columns-from-annotations-and-labels branch from 76cc5e3 to 4fe59fe Compare July 29, 2019 11:01
@ingvagabund
Copy link
Copy Markdown
Member Author

ingvagabund commented Jul 29, 2019

@Fedosin thanks for the review. Once Azure PR is merged (where we are deciding which annotations/labels we want to use), we can distribute the same change to other actuators.

@ingvagabund
Copy link
Copy Markdown
Member Author

ingvagabund commented Jul 29, 2019

Updating machine and machineset types in openshift/cluster-api first: openshift/cluster-api#55

enxebre
enxebre reviewed Jul 29, 2019
View reviewed changes
Comment thread install/0000_30_machine-api-operator_02_machine.crd.yaml Outdated
@ingvagabund
Sync with github.com/openshift/cluster-api
205f43a
@ingvagabund
Regenerate machine CRD
4542417 
Given we already support AWS, Azure and GCP instalations of OpenShift,
reading values from machine provider config is no longer an option.
Instead, let's read all needed values from machine annotations and labels.
Each actuator will be then responsible for making sure every machine
has all the annotations and labels that are needed to properly display
additional machine columns set.
@ingvagabund ingvagabund force-pushed the get-machine-additional-columns-from-annotations-and-labels branch from 4fe59fe to 4542417 Compare August 5, 2019 11:47
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 5, 2019
@ingvagabund ingvagabund mentioned this pull request Aug 5, 2019
Closed
@ingvagabund
Copy link
Copy Markdown
Member Author

ingvagabund commented Aug 7, 2019

/retest

@enxebre
Copy link
Copy Markdown
Member

enxebre commented Aug 7, 2019

/approve

@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Aug 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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 Aug 7, 2019
@spangenberg
Copy link
Copy Markdown
Contributor

spangenberg commented Aug 8, 2019

/lgtm

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

ingvagabund commented Aug 8, 2019 •
edited
Loading

/hold cancel

Just validated OpenShift in both AWS and Azure properly display all relevant columns when running oc get machines -n openshift-machine-api.

@ingvagabund ingvagabund removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 8, 2019
abhinavdahiya
abhinavdahiya reviewed Aug 8, 2019
View reviewed changes
Comment thread install/0000_30_machine-api-operator_02_machine.crd.yaml
name: Node
priority: 1
type: string
- JSONPath: .spec.providerID
Copy link
Copy Markdown
Contributor

@abhinavdahiya abhinavdahiya Aug 8, 2019

Choose a reason for hiding this comment

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

Should this be from the status??

Copy link
Copy Markdown
Member Author

@ingvagabund ingvagabund Aug 8, 2019

Choose a reason for hiding this comment

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

ProviderID is spec field and there was a long discussion which of spec or status is the right place to put it. Spec won.

@openshift-merge-robot openshift-merge-robot merged commit 8c38a49 into openshift:master Aug 8, 2019
@ingvagabund ingvagabund deleted the get-machine-additional-columns-from-annotations-and-labels branch August 8, 2019 14:39
@Fedosin
Copy link
Copy Markdown
Contributor

Fedosin commented Aug 16, 2019

@ingvagabund hi! add the OpenStack PR please openshift/cluster-api-provider-openstack#55

@ingvagabund
Copy link
Copy Markdown
Member Author

ingvagabund commented Aug 16, 2019 •
edited
Loading

@Fedosin there is no need to update machine CRD anymore. The CRD expects certain annotations and labels to be present in machine CR. If they are not, corresponding column is just empty. So, the only step needed to is update an actuator to set the requires annotations/labels.

@Fedosin
Copy link
Copy Markdown
Contributor

Fedosin commented Aug 16, 2019

@ingvagabund correct. that's what I did in openshift/cluster-api-provider-openstack#55

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

@bison bison Awaiting requested review from bison

@frobware frobware Awaiting requested review from frobware

+2 more reviewers

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@Fedosin Fedosin Fedosin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@spangenberg spangenberg

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.

7 participants

@ingvagabund @Fedosin @enxebre @openshift-ci-robot @spangenberg @abhinavdahiya @openshift-merge-robot