-
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
✨ Add generic addon deployment config to tilt #6991
✨ Add generic addon deployment config to tilt #6991
Conversation
49721f2
to
efe9632
Compare
ecf8edb
to
6972b86
Compare
44448aa
to
ec8a8bf
Compare
214546b
to
1e9ff79
Compare
1e9ff79
to
811037e
Compare
11562b0
to
0c4ca17
Compare
fb202de
to
bfd2d1a
Compare
8180a69
to
1abe060
Compare
f076064
to
20bd979
Compare
3f3e76e
to
53ba52b
Compare
hack/tools/tilt-prepare/main.go
Outdated
BinaryName string `json:"binary_name,omitempty"` | ||
Context *string `json:"context"` | ||
} | ||
|
||
type providerSettings struct { | ||
Name string `json:"name,omitempty"` | ||
Context *string `json:"context"` |
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.
Q: (possibly follow-up): I think Context here also has to be below a config
element.
cf.:
vSphere:
{
"name": "vsphere",
"config": {
"image": "gcr.io/cluster-api-provider-vsphere/release/manager",
"live_reload_deps": [...],
"label": "CAPV"
}
}
Tiltfile:
"core": {
"context": ".",
"image": "gcr.io/k8s-staging-cluster-api/cluster-api-controller",
"live_reload_deps": [...],
"label": "CAPI",
}
Context seems to be on the same level as live_reload_deps
and thus should be below config
.
Just noticed when reviewing addonSettings.
c369b1b
to
da235b3
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.
We're getting very close. Mostly just nits/cleanup
return runTaskGroup(ctx, "resources", tasks) | ||
} | ||
|
||
func readAddonSettings(path string) (*addonSettings, error) { | ||
path, err := checkWorkloadFileFormat(path, "addon") |
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.
I wonder if we should also allow arrays here like in the provider case to allow defining multiple addons per repository
Also totally up to you (or possibly follow-up PR)
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.
I think we should leave that to a follow up if it's needed BTW - is there a provider that uses it this way today?
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.
Afaik not. But I think for extensions/addons it makes sense to support more than one extension per repository.
I can take the follow-up, it's a relatively simple change. Making it relatively soon avoids that folks have to change their config later.
7271fab
to
405f166
Compare
405f166
to
e2652eb
Compare
Signed-off-by: killianmuldoon <[email protected]>
e2652eb
to
9929d16
Compare
/lgtm Looks great! We'll address the open findings in a follow-up PR. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
/hold cancel Wrong command at #6991 (comment) 🙂 |
Signed-off-by: killianmuldoon [email protected]
Add a generic deployment mechanism for addons to the tilt file and tilt prepare. This mechanism allows Runtime SDK extensions to be developed alongside the core controllers using the Tilt development flow. It can also be used to develop arbitrary deployments and addons built using the same workflow in tilt as providers use today.
May be related to #6647
/area runtime-sdk
Fixes #6864