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
Removing default for KUBECONFIG to enable incluster e2e tests #2191
Removing default for KUBECONFIG to enable incluster e2e tests #2191
Conversation
@salaboy: The label(s) 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. |
Welcome @salaboy! It looks like this is your first PR to knative/pkg 🎉 |
Hi @salaboy. Thanks for your PR. I'm waiting for a knative 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/test-infra repository. |
@markusthoemmes I will appreciate feedback on this one, as I am not entirely sure to understand the comments above the if statement. Is there a reason why tests cannot be run incluster? Because this check here is definitely blocking that possibility, unless there is a way to bypass that check. |
/assign @coryrc |
// and production code and we want to make sure that production code defaults to | ||
// the in-cluster config correctly. | ||
if f.Kubeconfig == "" { | ||
f.Kubeconfig = clientcmd.RecommendedHomeFile |
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.
doesn't this mean that this will no longer properly default the flag when running locally?
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.
So, checking the code, I think this should work, as the fallback is precisely this path as well.
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.
yeah, looks like the fallback was added after this code, which explains why it was added, and why it's now redundant 🤞
/ok-to-test |
Codecov Report
@@ Coverage Diff @@
## main #2191 +/- ##
==========================================
- Coverage 67.58% 66.09% -1.50%
==========================================
Files 215 221 +6
Lines 9098 9257 +159
==========================================
- Hits 6149 6118 -31
- Misses 2675 2863 +188
- Partials 274 276 +2
Continue to review full report at Codecov.
|
@julz @markusthoemmes tests seems to be failing but unrelated with the change.. I will figure out how to reproduce that.. or should we just retrigger the tests |
re-ran and the unit tests ran fine /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, salaboy 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 |
Changes
Removing default setting to KUBECONFIG in e2e flags. This allows e2e testing to be done from inside a Kubernetes Cluster.
/kind bug
/kind cleanup
Fixes #2190
closes #2190