-
Notifications
You must be signed in to change notification settings - Fork 234
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
New Addons API #1462
New Addons API #1462
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kron4eg 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 |
dc82c6f
to
9ac7add
Compare
/test pull-kubeone-build |
5e840cd
to
ed1eff2
Compare
@@ -198,7 +198,7 @@ func TestEnsureAddonsLabelsOnResources(t *testing.T) { | |||
LocalFS: os.DirFS(addonsDir), | |||
} | |||
|
|||
manifests, err := applier.loadAddonsManifests(applier.LocalFS, ".", nil, false, "") | |||
manifests, err := applier.loadAddonsManifests(applier.LocalFS, ".", nil, nil, false, "") |
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 would be nice if we would have a test case to test manifest parsing when a map is provided.
pkg/apis/kubeone/v1beta1/types.go
Outdated
// Delete flag to ensure the named addon with all its contents to be deleted | ||
Delete bool `json:"deleted,omitempty"` |
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.
Is this Delete
implemented at all?
GlobalParams map[string]string `json:"globalParams,omitempty"` | ||
|
||
// Addons is a list of config options for named addon | ||
Addons []Addon `json:"addons,omitempty"` |
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.
Have you implemented enabling embedded addons via this API yet?
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.
No, this is out of scope.
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.
This is actually the key part of the Epic. Passing parameters to addons was supposed to be only a nice addition if we have time left, but not the key part. Quoting part of the Epic description:
We need to implement an API that can be used to enable optional addons embedded in the binary, such as
cluster-autoscaler
andunattended-upgrades
.
Can you please extend the PR to handle this as well? I think it should be fairly easy, but it might take some time to test it properly.
024a56f
to
2534496
Compare
Signed-off-by: Artiom Diomin <kron82@gmail.com>
Signed-off-by: Artiom Diomin <kron82@gmail.com>
Signed-off-by: Artiom Diomin <kron82@gmail.com>
Signed-off-by: Artiom Diomin <kron82@gmail.com>
/lgtm |
LGTM label has been added. Git tree hash: 7a109b4ef3c20357ce04056b1caa09a9e84e59ec
|
/retest Review the full test history Silence the bot with an Also, here is a cat. |
In response to this:
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. |
What this PR does / why we need 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):xref #1459
Special notes for your reviewer:
Does this PR introduce a user-facing change?: