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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to CRDs - WIP #67

Merged
merged 20 commits into from
Oct 19, 2018
Merged

Migrate to CRDs - WIP #67

merged 20 commits into from
Oct 19, 2018

Conversation

enxebre
Copy link
Member

@enxebre enxebre commented Oct 11, 2018

Migrate to CRDs:

  • New api generation and layout based on kubebuilder
    -- kubebuilder init --domain k8s.io --license apache2 --owner “The Kubernetes Authors”
    -- kubebuilder create api --group awsproviderconfig --version v1alpha1 --kind AWSMachineProviderConfig
  • Move the cloud/ code from root folder into pkg/cloud
  • revendor
  • Fix machine status, provider status
  • Update tests/ and cmd/aws-actuator
  • Update docker file and Make

How to run it:
kustomize build vendor/sigs.k8s.io/cluster-api/config/default/ >> cluster-api-bootstrap.yaml
kubectl apply -f cluster-api-bootstrap.yaml
make manager
./bin/manager

How to regenerate code:
go generate ./pkg/... ./cmd/...

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

@frobware frobware left a comment

Choose a reason for hiding this comment

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

I took a very brief look at this; clearly a lot of WIP and just a couple of comments for now.

log.Printf("Starting the Cmd.")

// Start the Cmd
log.Fatal(mgr.Start(signals.SetupSignalHandler()))

Choose a reason for hiding this comment

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

If SIGTERM is received does Start() return? If so, I wouldn't expect that to print a fatal error.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2018
@enxebre enxebre added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 16, 2018
frobware and others added 11 commits October 16, 2018 17:52
Invoking:

    $ make build DBG=1

or

    $ make aws-actuator DBG=1

will build cmd/cluster-controller, cmd/machine-controller and
bin/aws-actuator with Go debug support. The build rules for those
three binaries are now `go build $(GOGCFLAGS) ...`.

The default value for GOGCFLAGS is -gcflags=all="-N -l", assuming
GOGCFLAGS it not already set in your environment. "all" specifies that
the flags should be applied to all packages (and may rebuild existing
up to date packages); "-N" disables compiler optimisations and "-l"
prevents inlining.

So, to override the defaults:

    $ make build DBG=1 GOGCFLAGS='-gcflags="-N -l"'
Makefile: add Go debug support for local binaries
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2018
@enxebre enxebre changed the base branch from master to crd October 17, 2018 11:17
@enxebre enxebre changed the base branch from crd to master October 17, 2018 11:21
@enxebre enxebre force-pushed the crds branch 2 times, most recently from 60e5b11 to 320f710 Compare October 17, 2018 13:50
@enxebre
Copy link
Member Author

enxebre commented Oct 17, 2018

/test images

log.Printf("Registering Components.")

// Setup Scheme for all resources
if err := apis.AddToScheme(mgr.GetScheme()); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This is nice.

package controller

import (
"sigs.k8s.io/cluster-api-provider-aws/migration/pkg/controller/awsmachineproviderconfig"
Copy link
Member

Choose a reason for hiding this comment

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

Where can I find this migration dir?

)

func init() {
pflag.CommandLine.StringVar(&logLevel, "log-level", defaultLogLevel, "Log level (debug,info,warn,error,fatal)")
Copy link
Member

Choose a reason for hiding this comment

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

We should use flag since sigs.k8s.io/controller-runtime defines its own flags which gets ignored by pflag.

const (
// ClusterNameLabel is the label that a machineset must have to identify the
// cluster to which it belongs.
ClusterNameLabel = "sigs.k8s.io/cluster-api-cluster"
Copy link
Member

Choose a reason for hiding this comment

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

Are those labels community defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is a carry over, there's bunch of things to consolidate after this. Let's address any non specific to CRDs move in different follow ups

@ingvagabund
Copy link
Member

kubernetes-sigs/cluster-api#546 is definitely blocker

@enxebre enxebre changed the base branch from master to crd October 18, 2018 16:15
@enxebre
Copy link
Member Author

enxebre commented Oct 18, 2018

/test e2e

@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 18, 2018

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

Test name Commit Details Rerun command
ci/prow/e2e-aws 7bc2a82 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.

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
Copy link
Member

/retest

@ingvagabund
Copy link
Member

/test e2e

@ingvagabund
Copy link
Member

/test e2e

@ingvagabund
Copy link
Member

/approve

@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 Oct 19, 2018
@ingvagabund
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2018
@openshift-merge-robot openshift-merge-robot merged commit 261db4a into openshift:crd Oct 19, 2018
vikaschoudhary16 pushed a commit to vikaschoudhary16/cluster-api-provider-aws that referenced this pull request Jan 16, 2019
* Adds simple Exists implementation to machine actuator

Remove the machine actuator test for now.

Trying to test it will be possible, but it will be a *ton* of work and
right now it's not worth the effort.

Exports the ec2 service's EC2 field.

Signed-off-by: Chuck Ha <chuck@heptio.com>

* Update deps

Signed-off-by: Chuck Ha <chuck@heptio.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants