-
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
Replace kubectl apply
in e2e test framework
#9696
Comments
My preference for a quick win is to just replace the If somebody specifically wants the apply functionality in the e2e framework it would be a good opportunity to contribute. |
/triage accepted |
At a first glance we should get rid of all the os.exec kubectl stuff (not sure if there's more than apply) |
There's also a |
A first implementation can be found here: #9695 /help |
@sbueringer: GuidelinesPlease ensure that the issue body includes answers to the following questions:
For more details on the requirements of such an issue, please see here and ensure that they are met. If this request no longer meets these requirements, the label can be removed 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. |
@sbueringer @killianmuldoon I can take this issue. |
Thx! /assign adilGhaffarDev |
👋 @killianmuldoon touching back on this one -- should this issue still be pursued? i see one of the PRs to fix was closed with a comment indicating a separate PR was merged that covered this: |
I think if we find a good way to get rid of the os.exec with kubectl that would be good |
/kind cleanup |
/assign (DMed you about this awhile ago @adilGhaffarDev taking a stab at this one) |
/assign cahillsf |
@sbueringer 👋 i have a draft open for this (sloppy right now but want to make sure im going in the right direction) the current signature accepts this the current logic of my update:
is this an approach that makes sense? if so I was thinking of updating the signature to something like: is there any reason we would need to support other |
Sounds good! |
In the CAPI e2e test framework currently the
kubectl
binary is called to apply yamls when creating Clusters. This would improve error reporting from this function which may help to debug #9688.cluster-api/test/framework/cluster_proxy.go
Lines 248 to 254 in 2caa534
The easiest solution is to create objects in the test framework with a
client.Create
call either changing the underlying function. This is the approach in #9077. We can ignorealreadyExists
errors to make the call retryable.Alternatively we could use the actual rest call from
kubectl
which has the benefit of actually being an 'apply' call. The only issue with this is needing to import thek8s.io/kubectl
library and it being relatively complicated to implement.The text was updated successfully, but these errors were encountered: