Skip to content

Api pivot#150

Closed
vikaschoudhary16 wants to merge 2 commits intoopenshift:masterfrom
vikaschoudhary16:api-pivot
Closed

Api pivot#150
vikaschoudhary16 wants to merge 2 commits intoopenshift:masterfrom
vikaschoudhary16:api-pivot

Conversation

@vikaschoudhary16
Copy link

@vikaschoudhary16 vikaschoudhary16 commented Feb 5, 2019

Add awsproviderconfig under oepnshift.io group

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 5, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 5, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vikaschoudhary16

If they are not already assigned, you can assign the PR to them by writing /assign @vikaschoudhary16 in a comment when ready.

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

@enxebre
Copy link
Member

enxebre commented Feb 5, 2019

This seems to be introducing a lot of API changes.
Can we split this in different PRs? One for pivoting the API group. Then follow ups for the explicit API changes
This needs openshift/installer#1175 to get in so then we can update the e2e-operator tests here to run against machine.openshift.io group and merge this safely
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 5, 2019
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 6, 2019
@vikaschoudhary16 vikaschoudhary16 force-pushed the api-pivot branch 2 times, most recently from 4efec09 to 87c5e39 Compare February 6, 2019 10:16
@ingvagabund
Copy link
Member

ingvagabund commented Feb 7, 2019

@vikaschoudhary16 I don't think we need to s/awsproviderconfig/openshiftawsproviderconfig the package name. It's sufficient to just change s/awsproviderconfig.k8s.io/awsproviderconfig.openshift.io.

@vikaschoudhary16
Copy link
Author

vikaschoudhary16 commented Feb 7, 2019

@ingvagabund group name is awsprovider.openshift.io. Directory name is including openshift because initially i had upstream apis in awsprovider dir at same path. Do we think that we will never have both upstream and openshift apis at this path?

@ingvagabund
Copy link
Member

ingvagabund commented Feb 7, 2019

@ingvagabund group name is awsprovider.openshift.io. Directory name is including openshift because initially i had upstream apis in awsprovider dir at same path. Do we think that we will never have both upstream and openshift apis at this path?

If we will, we can always do it later. And most likely we would run two actuators, each for one group.

@vikaschoudhary16 vikaschoudhary16 force-pushed the api-pivot branch 3 times, most recently from 38fee92 to 69668ad Compare February 7, 2019 11:44
enxebre
enxebre reviewed Feb 7, 2019
View reviewed changes
pkg/actuators/machine/actuator.go Outdated
Copy link
Member

@enxebre enxebre Feb 7, 2019

Choose a reason for hiding this comment

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

Can you add to do here explaining why is needed?

Copy link
Member

@ingvagabund ingvagabund Feb 7, 2019

Choose a reason for hiding this comment

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

Instead of replacing a.codec.DecodeProviderStatus with yaml.Unmarshal, let's replace the code in DecodeProviderStatus itself [1].

[1] https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/apis/awsproviderconfig/v1alpha1/register.go#L110

enxebre
enxebre reviewed Feb 7, 2019
View reviewed changes
pkg/actuators/machine/actuator.go Outdated
Copy link
Member

@enxebre enxebre Feb 7, 2019

Choose a reason for hiding this comment

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

Can you add to do here explaining why is needed?

enxebre
enxebre reviewed Feb 7, 2019
View reviewed changes
pkg/actuators/machine/utils.go Outdated
Copy link
Member

@enxebre enxebre Feb 7, 2019

Choose a reason for hiding this comment

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

Can you add to do here explaining why is needed?

Copy link
Author

@vikaschoudhary16 vikaschoudhary16 Feb 7, 2019

Choose a reason for hiding this comment

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

sure

Copy link
Member

@ingvagabund ingvagabund Feb 7, 2019

Choose a reason for hiding this comment

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

The same here. Please update DecodeProviderSpec itself [1] rather than replacing DecodeProviderSpec.

[1] https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/apis/awsproviderconfig/v1alpha1/register.go#L79

@vikaschoudhary16 vikaschoudhary16 force-pushed the api-pivot branch 3 times, most recently from f92c014 to ee6e31a Compare February 7, 2019 14:50
@ingvagabund
Copy link
Member

ingvagabund commented Feb 7, 2019

/test e2e

ingvagabund
ingvagabund requested changes Feb 7, 2019
View reviewed changes
pkg/actuators/machine/actuator.go Outdated
Copy link
Member

@ingvagabund ingvagabund Feb 7, 2019

Choose a reason for hiding this comment

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

Instead of replacing a.codec.DecodeProviderStatus with yaml.Unmarshal, let's replace the code in DecodeProviderStatus itself [1].

[1] https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/apis/awsproviderconfig/v1alpha1/register.go#L110

pkg/actuators/machine/utils.go Outdated
Copy link
Member

@ingvagabund ingvagabund Feb 7, 2019

Choose a reason for hiding this comment

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

The same here. Please update DecodeProviderSpec itself [1] rather than replacing DecodeProviderSpec.

[1] https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/apis/awsproviderconfig/v1alpha1/register.go#L79

@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 7, 2019

@vikaschoudhary16: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/gofmt 1ac11a4 link /test gofmt
ci/jenkins/e2e 1ac11a4 link /test e2e
ci/prow/e2e-aws-operator 1ac11a4 link /test e2e-aws-operator
ci/prow/e2e-aws 1ac11a4 link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ingvagabund ingvagabund mentioned this pull request Feb 7, 2019
Merged
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 9, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 9, 2019

@vikaschoudhary16: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ingvagabund
Copy link
Member

ingvagabund commented Feb 22, 2019

Closing in favor of #152. Thank you @vikaschoudhary16 for the PR.

michaelgugino pushed a commit to mgugino-upstream-stage/cluster-api-provider-aws that referenced this pull request Feb 12, 2020
@vincepri @k8s-ci-robot
Add region cluster configuration parameter (openshift#150)
8101679 
* Add region cluster configuration parameter

Signed-off-by: Vince Prignano <vince@vincepri.com>

* pass region in status

Signed-off-by: Vince Prignano <vince@vincepri.com>
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 requested changes

@paulfantom paulfantom Awaiting requested review from paulfantom

Assignees

No one assigned

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vikaschoudhary16 @openshift-ci-robot @enxebre @ingvagabund