-
Notifications
You must be signed in to change notification settings - Fork 129
dev: add tiltfile for local development #1594
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
base: main
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3b056e0 to
121767c
Compare
tuminoid
left a comment
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.
Default images run as non-root and with other security hardenings, which should prevent Tilt from updating the manager binaries when they change. I don't see that patched here, is it now handled differently? Ref: https://github.com/metal3-io/cluster-api-provider-metal3/blob/11189599fd7b84280e8c1ce0f56d44154cb86d80/Tiltfile#L4
I'm not sure I understand what you mean, can you elaborate ? |
No, I did not test this, just noticed its different on this part. If you check the line referenced, it calls remove_sec_ctx.py which removes the securityContext from the yaml, to allow Tilt to update the manager binary on the running containers. It could be Tilt has been updated to workaround this then? Or is it deployed some other more permissive way? |
Not sure to be honest. I'm trying to understand why would Tilt not be able to deploy k8s objects if they have some |
SecurityContext might have |
As discussed offline, let's leave this kind of build process optimization for later as follow up. |
And just for completeness sake, this is not an issue in the current form of PR as it does full container rebuild instead of binary replacement. This support needs to be brought back later as it is making development cycle faster by avoiding full redeployment. We also have some other custom parts (like BMH creation button) that need to back added later. |
Signed-off-by: Feruzjon Muyassarov <[email protected]>
121767c to
4608cbf
Compare
|
For the docs, I wonder whether it’s better to add the details in the root markdown instead of in a separate document, which may never be read or may be easily overlooked. |
lentzi90
left a comment
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 it is good with a separate doc, but you may want to link to it from the main README.md.
A couple of things out of sync between code and docs below. Otherwise this looks ok to me. I have not tested it though.
|
|
||
| - Check that all required tools are installed | ||
| - Clone repositories to `./local-dev` (only if they don't exist) | ||
| - Deploy cert-manager (by default v1.3.1) |
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.
| - Deploy cert-manager (by default v1.3.1) | |
| - Deploy cert-manager (by default v1.19.1) |
|
Ah the Nordix comment still shows as resolved. Commenting here for visibility. The Tiltfile now uses metal3-io but the doc still has Nordix in one place. |
This is initial version which is expected to iterate over to have more customization points such as, disabling auto update or continuing on top of metal3-dev-env prepared environment. Right now, it doesn't have and dependency nor connection to the way that metal3-dev-env prepares the dev environment but I plan to make it easier to integrate it in case developers want to continue modifying and testing their metal3 controllers on the environment prepared by metal3-dev-env (optionally).