Add support for extensions in the deploy-to-kube script#204
Add support for extensions in the deploy-to-kube script#204igooch wants to merge 3 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @igooch. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
k8s-ci-robot
added
size/L
✅ Deploy Preview for agent-sandbox canceled.
|
|
|
||
| def main(args): | ||
| repo_root = utils.get_repo_root() | ||
| def gather_manifests(manifests_path): |
There was a problem hiding this comment.
For some reason I feel the original implementation is cleaner? Do we need to spilt it into 3 functions? For readability, maybe we can just add a comment on top of the main function to describe what the code was doing and regard that as enough?
Or if you have a reason for why splitting it into 3 functions is better?
There was a problem hiding this comment.
I find this new approach very nice, the functions are self-documenting and they also have extensive function documents. I also hope that we might be able to move more of the functions into shared helpers, so this feels like a good step towards that.
I think that you should compare the original implementation with support for extensions to the new code. I can see how once we add extensions the original implementation will get messier.
Personally I think it's a good step in the right direction, I'd like to see e.g. whether we could make process_manifests more generic, but that's not a blocker for this work, more just my (personally) preferred direction for the future.
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| continue | ||
|
|
||
| # When extensions is enabled, we use a the extensions controller manifest instead. | ||
| if args.extensions and filename == "controller.yaml" and kind == "StatefulSet": |
There was a problem hiding this comment.
This is slightly brittle if the file is renamed or if multiple controller files exist. This also assumes file names follow certain convention (e.g. start with extensions). Also, we might change the controller to a Deployment in #191.
Also, we plan to change controller manifests in core/extensions in the future: #192
There was a problem hiding this comment.
I updated to use the metadata name instead of the file name. Ready for re-review.
…ntroller with extensions installed
igooch
force-pushed
the
deploy-extensions
branch
from
caeea0f to
cfc3d78
Compare
|
Looks good to me! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: igooch, janetkuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@igooch would you take a look at the test failures? |
|
@igooch: 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. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
@igooch please reassign to me after the test failure is fixed |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to us at Agent Sandbox. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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-sigs/prow repository. |
|
is this still valid (required) ? @igooch ? |
|
Closing this out since it is stale |
|
/close |
|
@aditya-shantanu: Closed this PR. DetailsIn 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-sigs/prow repository. |
8 participants
The
deploy-to-kubescript is a critical tool for development, but its initial implementation was too rigid, making it difficult to deploy different controller configurations, such as enabling the extension controllers. The script lacked the ability to selectively apply manifests, and its monolithic structure made it hard to maintain and extend.This PR addresses these issues by refactoring the
deploy-to-kubescript to be more modular, readable, and allow for deploying the controller with extensions enabled.Key Changes:
Notes for reviewer:
To deploy to the agent-sandbox controller with extensions enabled onto your cluster using your local development code run:
./dev/tools/push-images --image-prefix={YOUR_REPO_PATH/}and then./dev/tools/deploy-to-kube --image-prefix={YOUR_REPO_PATH/} --extensions.Alternatively, use
EXTENSIONS=true make deploy-kindto create and deploy onto a Kind cluster.Fixes #120