search

kubernetes-sigs / krew

📦 Find and install kubectl plugins
https://krew.sigs.k8s.io
Apache License 2.0
6.42k stars 369 forks source link

Fix tests for users that set KREW_ROOT #783

Closed avorima closed 1 year ago

avorima commented 2 years ago

I've noticed that updating the krew AUR package failed and after looking into it I saw that the tests failed because I had set KREW_ROOT. This patch unsets KREW_ROOT for the test case that asserts the default path.

k8s-ci-robot commented 2 years ago

Welcome @avorima!

It looks like this is your first PR to kubernetes-sigs/krew 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/krew has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

avorima commented 2 years ago

/retest

k8s-ci-robot commented 2 years ago

@avorima: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to [this](https://github.com/kubernetes-sigs/krew/pull/783#issuecomment-1133609765): >/retest Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
JohnTitor commented 2 years ago

@avorima CI issue has been fixed, could you rebase?

avorima commented 2 years ago

@JohnTitor done

ahmetb commented 2 years ago

I am -1 on this since it changes the behavior for the rest of the test suite by using os.Unsetenv. If only this particular test case is impacted, we should implement something that preserves the old value and whether it was set at all. Right now it just removes the env entry for all unit tests, which might cause trouble down the road.

k8s-ci-robot commented 2 years ago

New changes are detected. LGTM label has been removed.

avorima commented 2 years ago

@ahmetb I added a helper that sets and unsets KREW_ROOT, because the envOverride test case also modifies that value.

ahmetb commented 2 years ago

I'm still not entirely sure why you need this.

  1. why are you setting KREW_ROOT during a test suite?
  2. why is https://pkg.go.dev/testing#t.Setenv not doing what you want? you basically reimplemented it.
avorima commented 2 years ago

When KREW_ROOT is set one (1) test case fails, because it is not expected to be set which is why it must be unset. This is the only fix that I want to get through. Another one (1) test case expects KREW_ROOT to have a value, so it must be set for that one (1) test case. So to ensure that the os.Unsetenv is only active for one test case (like you asked), I added the helper which sets and restores the value of KREW_ROOT.

So the tests expect KREW_ROOT to have certain values at certain times and I'm just trying to make things work when it is set externally, i.e. by using export in the shell instead of os.Setenv.

k8s-triage-robot commented 2 years ago

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

avorima commented 2 years ago

/remove-lifecycle stale

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot commented 1 year ago

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 sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot commented 1 year ago

@k8s-triage-robot: Closed this PR.

In response to [this](https://github.com/kubernetes-sigs/krew/pull/783#issuecomment-1516257171): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages PRs according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the PR is closed > >You can: >- Reopen this PR with `/reopen` >- Mark this PR as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ahmetb commented 1 year ago

/reopen /lgtm /approve

k8s-ci-robot commented 1 year ago

@ahmetb: Reopened this PR.

In response to [this](https://github.com/kubernetes-sigs/krew/pull/783#issuecomment-1516527228): >/reopen >/lgtm >/approve Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, avorima

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [ahmetb] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
k8s-triage-robot commented 1 year ago

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

You can:

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot commented 1 year ago

@k8s-triage-robot: Closed this PR.

In response to [this](https://github.com/kubernetes-sigs/krew/pull/783#issuecomment-1555942652): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages PRs according to the following rules: >- After 90d of inactivity, `lifecycle/stale` is applied >- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied >- After 30d of inactivity since `lifecycle/rotten` was applied, the PR is closed > >You can: >- Reopen this PR with `/reopen` >- Mark this PR as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.