Skip to content
This repository has been archived by the owner on May 22, 2020. It is now read-only.

Standardize ProviderConfig for Cluster and Machine #672

Merged

Conversation

craigtracey
Copy link
Contributor

What this PR does / why we need it:
In order to maintain type parity for providerConfig across both Machine
and Cluster types, change the ClusterSpec.ProviderConfig to the
ProviderConfig type.

Additionally, as all changes to the API require regenerating code, add a
Makefile that will simplify that process.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

None

@kubernetes/kube-deploy-reviewers

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 7, 2018
In order to maintain type parity for providerConfig across both Machine
and Cluster types, change the ClusterSpec.ProviderConfig to the
ProviderConfig type.

Additionally, as all changes to the API require regenerating code, add a
Makefile that will simplify that process.

Signed-off-by: Craig Tracey <craigtracey@gmail.com>
@craigtracey craigtracey force-pushed the cluster-provider-config branch from 88b12e9 to bc2aab4 Compare April 7, 2018 03:56
@krisnova
Copy link
Contributor

krisnova commented Apr 7, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 7, 2018
@rsdcastro
Copy link

/assign krousey

cc @krousey

Copy link
Contributor

@krousey krousey left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. Just a couple of comments.


// No more than one of the following may be specified.

// Value is an inlined, serialized representation of the node
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "node" and replace appropriately.


import runtime "k8s.io/apimachinery/pkg/runtime"

// ProviderConfig defines the configuration to use during node creation.
Copy link
Contributor

Choose a reason for hiding this comment

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

"node"

all: genapi genconversion

genapi:
go get -u github.com/kubernetes-incubator/apiserver-builder/cmd/apiregister-gen
Copy link
Contributor

Choose a reason for hiding this comment

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

This means we're always using the tool at head. Is it possible to vendor, build from vendor, and use that copy?

apiregister-gen -i ./pkg/apis,./pkg/apis/cluster/v1alpha1

genconversion:
go get -u k8s.io/code-generator/cmd/conversion-gen
Copy link
Contributor

Choose a reason for hiding this comment

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

Same vendoring comment for this.

Modify the dep config to lock our codegen dependencies and make use of
these dependencies in a top-level Makefile.

Signed-off-by: Craig Tracey <craigtracey@gmail.com>
Updating vendor dependencies in response to introduction of
k8s.io/code-generator.

Signed-off-by: Craig Tracey <craigtracey@gmail.com>
As this code has moved from being Machine-specific to being generic,
remove references to "node" in favor of "resource." "resource" has been
chosen as this type may be leveraged by any type that would like to
implement "providerConfig" semantics.

Signed-off-by: Craig Tracey <craigtracey@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2018
@krousey
Copy link
Contributor

krousey commented Apr 10, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: craigtracey, krousey

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 10, 2018
@k8s-ci-robot k8s-ci-robot merged commit e131c0e into kubernetes-retired:master Apr 10, 2018
@jessicaochen jessicaochen mentioned this pull request Apr 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants