Skip to content
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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAPA v1alpha2 :tada: #892

Merged
merged 21 commits into from Jul 12, 2019

Conversation

@vincepri
Copy link
Member

commented Jul 11, 2019

Signed-off-by: Vince Prignano vincepri@vmware.com

What this PR does / why we need it:
This breaks the world as we know it 馃槃

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 #883

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:


@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

@vincepri vincepri force-pushed the vincepri:v1a2-types branch 2 times, most recently from 64443d0 to 30b3e46 Jul 11, 2019
@vincepri vincepri force-pushed the vincepri:v1a2-types branch from 30b3e46 to b7b7d91 Jul 11, 2019
@vincepri vincepri force-pushed the vincepri:v1a2-types branch 4 times, most recently from 033ce67 to 57b5ffa Jul 11, 2019
@vincepri vincepri force-pushed the vincepri:v1a2-types branch 10 times, most recently from e344066 to 0fc854b Jul 11, 2019
@vincepri

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Just a little heads up that this PR removes a lot of Bazel apart from the linters, which I agree with @detiber that those should be fixed to a specific version.

A few reasons:

  • Bazel custom files add unnecessary complexity.
  • Skylark is hard to maintain and read, there are almost no comment around.
  • We continue to get in trouble with Bazel, bazel_gazelle, etc. and we ended up running a git hash version because of compatibility issues.
  • Go tools are really solid and new contributors are already used to similar workflows.
  • Error messages when running Go tests or generating files through Bazel are cryptic, I personally always end up running plain old go tools to get to the actual error.

Let's keep things simple.

Copy link

left a comment

i must admit that i don't understand all the demands of this repository but from what i've seen in the k8s space, bazel does have some special benefits like shared cache (prow jobs) and fast compile times (once you download and lot of stuff and once it's done building once), but other than that i'm not convinced that anything else other than parst of k/test-infra and k/k need it.

in fact, i'd argue it's overkill to use it for jobs like hack/verify-config.sh in test-infra.

so +1 on not using bazel here.
(some might disagree).

@justaugustus

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

+1 to removing as much bazel mysticism as possible!

@vincepri vincepri closed this Jul 12, 2019
vincepri added 8 commits Jul 12, 2019
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Signed-off-by: Vince Prignano <vincepri@vmware.com>
@vincepri vincepri force-pushed the vincepri:v1a2-types branch from ed0f8aa to eb10b16 Jul 12, 2019
Signed-off-by: Vince Prignano <vincepri@vmware.com>
@vincepri vincepri force-pushed the vincepri:v1a2-types branch from bc81d01 to c185cf4 Jul 12, 2019
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Copy link
Member

left a comment

that was a lot easier with commits, thanks for including individual commits.

A few comments, looks good to me tho

if err != nil {
return errors.Wrapf(err, "failed to list machines for cluster %q", cluster.Name)
}
// machines, err := a.client.Machines(cluster.Namespace).List(actuators.ListOptionsForCluster(cluster.Name))

This comment has been minimized.

Copy link
@chuckha

chuckha Jul 12, 2019

Member

any particular reason this was commented and not deleted?

This comment has been minimized.

Copy link
@vincepri

vincepri Jul 12, 2019

Author Member

Needs to be fixed in a future iteration

Logger logr.Logger
Cluster *clusterv1.Cluster
Machine *clusterv1.Machine
AWSMachine *v1alpha2.AWSMachine

This comment has been minimized.

Copy link
@chuckha

chuckha Jul 12, 2019

Member

I'm concerned scope is becoming a god object. it's so convenient that it gets passed around through the entire stack. Probably not worth a refactor here, but we should definitely keep an eye on this.

sigs.k8s.io/yaml v1.1.0
)

replace k8s.io/apimachinery => k8s.io/apimachinery v0.0.0-20190221213512-86fb29eff628
replace (

This comment has been minimized.

Copy link
@chuckha

chuckha Jul 12, 2019

Member

Why do you need this replace directive? Does putting these versions in the require directive not work? At the very least, the cluster-api version number is the same so that's doing nothing and could be removed

This comment has been minimized.

Copy link
@vincepri

vincepri Jul 12, 2019

Author Member

I started to document this here https://hackmd.io/cp5fIxAeQrullF61_U3PEw, I can remove cluster-api now, but in the beginning it actually always matched v1alpha1 because of the imports

@@ -164,29 +161,30 @@ func makeCluster() *capi.Cluster {
}

func makeMachine() *capi.Machine {
yaml, err := ioutil.ReadFile(*machineYAML)
Expect(err).To(BeNil())
// yaml, err := ioutil.ReadFile(*machineYAML)

This comment has been minimized.

Copy link
@chuckha

chuckha Jul 12, 2019

Member

maybe delete instead of comment?

This comment has been minimized.

Copy link
@vincepri

vincepri Jul 12, 2019

Author Member

I commented it out because it should be fixed in another iteration, it's helpful to keep this around for reference, same happened in CAPI before the logic went in

@@ -0,0 +1,111 @@
#!/usr/bin/env bash
# Copyright 2018 The Kubernetes Authors.

This comment has been minimized.

Copy link
@ncdc

ncdc Jul 12, 2019

Contributor

2019 or was this copied from elsewhere?

This comment has been minimized.

Copy link
@vincepri

vincepri Jul 12, 2019

Author Member

Copied from CAPI

Makefile Outdated Show resolved Hide resolved
Dockerfile Outdated
@@ -0,0 +1,36 @@
# Copyright 2018 The Kubernetes Authors.

This comment has been minimized.

Copy link
@ncdc

ncdc Jul 12, 2019

Contributor

2019 or was this copied from elsewhere?

This comment has been minimized.

Copy link
@vincepri

vincepri Jul 12, 2019

Author Member

Copied from CAPI

type AWSMachineStatus struct {
// InstanceID is the instance ID of the machine created in AWS
// +optional
InstanceID *string `json:"instanceID,omitempty"`

This comment has been minimized.

Copy link
@ncdc

ncdc Jul 12, 2019

Contributor

Will it be possible to redetermine the InstanceID from just the spec?

This comment has been minimized.

Copy link
@vincepri

vincepri Jul 12, 2019

Author Member

I'm not sure what you mean, I copied this from the providerStatus

pkg/apis/infrastructure/v1alpha2/register.go Outdated Show resolved Hide resolved
pkg/controller/awsmachine/awsmachine_controller.go Outdated Show resolved Hide resolved
pkg/controller/awsmachine/awsmachine_controller.go Outdated Show resolved Hide resolved
pkg/controller/awsmachine/awsmachine_controller.go Outdated Show resolved Hide resolved
Signed-off-by: Vince Prignano <vincepri@vmware.com>
@vincepri

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

@ncdc @chuckha ptal, I'll follow-up with different PRs to fix the rest

Signed-off-by: Vince Prignano <vincepri@vmware.com>
@vincepri vincepri force-pushed the vincepri:v1a2-types branch from 7819bbe to 64ed932 Jul 12, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

@vincepri: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-cluster-api-provider-aws-verify-boilerplate 0fc854b link /test pull-cluster-api-provider-aws-verify-boilerplate

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.

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.

Signed-off-by: Vince Prignano <vincepri@vmware.com>
@vincepri vincepri force-pushed the vincepri:v1a2-types branch from 548667f to 8007588 Jul 12, 2019
Signed-off-by: Vince Prignano <vincepri@vmware.com>
Copy link
Member

left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 12, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3268ab4 into kubernetes-sigs:master Jul 12, 2019
6 checks passed
6 checks passed
cla/linuxfoundation vincepri authorized
Details
pull-cluster-api-provider-aws-bazel-build Job succeeded.
Details
pull-cluster-api-provider-aws-bazel-integration Job succeeded.
Details
pull-cluster-api-provider-aws-bazel-test Job succeeded.
Details
pull-cluster-api-provider-aws-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.