-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🏃 allow extra args with tilt #2932
🏃 allow extra args with tilt #2932
Conversation
31dd04b
to
d83881e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/milestone v0.3.4
/assign @detiber
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexeldeib, 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexeldeib thanks for tackling this, a couple of questions related to the kubeadm-bootstrap
provider and the developer experience on the changes, but can't wait until we can start taking advantage of feature gates with the tilt-based workflow.
b6bbe92
to
8c4e53b
Compare
Co-Authored-By: Jason DeTiberus <[email protected]>
Co-Authored-By: Jason DeTiberus <[email protected]>
Co-Authored-By: Jason DeTiberus <[email protected]>
{ | ||
"extra_args": { | ||
"core": ["--feature-gates=MachinePool=true"], | ||
"kubeadm-bootstrap": ["--feature-gates=MachinePool=true"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this actually do anything? I'm not using it AFAIK, but I don't recall if kubeadm-bootstrap specifically uses this value at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it's used for adding the watch on MachinePool:
cluster-api/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go
Lines 109 to 116 in 99a95aa
if feature.Gates.Enabled(feature.MachinePool) { | |
builder = builder.Watches( | |
&source.Kind{Type: &expv1.MachinePool{}}, | |
&handler.EnqueueRequestsFromMapFunc{ | |
ToRequests: handler.ToRequestsFunc(r.MachinePoolToBootstrapMapFunc), | |
}, | |
) | |
} |
cluster-api/bootstrap/util/configowner.go
Lines 95 to 100 in 99a95aa
if feature.Gates.Enabled(feature.MachinePool) { | |
allowedGKs = append(allowedGKs, schema.GroupKind{ | |
Group: expv1.GroupVersion.Group, | |
Kind: "MachinePool", | |
}) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the links, probably should have tried a simple ctrl-f 😋
@alexeldeib: The following test failed, say
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. |
/test pull-cluster-api-capd-e2e |
/lgtm |
Allows extra args from the tilt provider config.
This makes testing things like feature flags much easier. It needs to be in provider config rather than settings so it can be used across all providers (settings files are 1 per invocation I think? but we could change that).