-
Notifications
You must be signed in to change notification settings - Fork 163
Add early cluster validation to prevent wasted build time in func deploy #3117
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
Signed-off-by: RayyanSeliya <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RayyanSeliya 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 |
|
Hi @RayyanSeliya. 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. 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3117 +/- ##
==========================================
+ Coverage 58.33% 59.15% +0.81%
==========================================
Files 134 134
Lines 17364 17512 +148
==========================================
+ Hits 10130 10359 +229
+ Misses 6293 6184 -109
- Partials 941 969 +28
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: RayyanSeliya <[email protected]>
|
/ok-to-test |
cmd/deploy.go
Outdated
|
|
||
| // validateClusterConnection checks if the Kubernetes cluster is accessible before starting build | ||
| func validateClusterConnection() error { | ||
| // Skip for test environments (check if using test kubeconfig) |
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 would prefer if we called our k8s.NewKubernetesClientset() and checked the err return value.
Also I do not like making the code "test aware". Could we think of something better?
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.
Thxs for the feedback @matejvasek ! I've updated it to use k8s.NewKubernetesClientset() and check the error as you suggested. The /testdata/ check follows Go's convention for test fixtures , it catches cases where tests use stale kubeconfig paths. The .example.com check skips API calls for test/example clusters (RFC 2606 reserved domain).
Signed-off-by: RayyanSeliya <[email protected]>
Signed-off-by: RayyanSeliya <[email protected]>
|
Im not much of a fan of this implementation... its still "test aware" as Matej said - with those literal strings and constants. Also there has to be some better way to implement this 🤔 |
@gauron99 Good point about the client architecture! I checked I'm thinking we could add: func (c *Client) ClusterAvailable(ctx context.Context) errorThen just call it before building. Tests would naturally skip it since they use The challenge is timing - we currently validate before creating the client (line 423 vs line 462 in
Which direction makes more sense for the existing code? Don't want to refactor the way which is not feasible ..! |
Changes
func deployWhat Changed
func deploynow validates Kubernetes cluster connectivity before starting the container build process, preventing wasted time when the cluster is inaccessible.Before:
--build=falsewas usedAfter:
Implementation
Added 2-layer error handling:
pkg/functions/errors.go): Technical errors (ErrInvalidKubeconfig,ErrClusterNotAccessible)cmd/deploy.go): User-friendly CLI messages with examplesDetects three distinct error scenarios:
Testing
Tested all combinations:
TestDeploy_ConfigPrecedence, etc.)/kind bug
Fixes #3116
Release Note