Skip to content
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

profile list --output json handle empty config folder #16900

Merged

Conversation

raghavendra-talur
Copy link
Contributor

fixes #15593

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2023
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 18, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @raghavendra-talur. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 18, 2023
@raghavendra-talur raghavendra-talur changed the title config: transform os.ENOENT errors into config.ErrNotExist config: transform os.ENOENT error into config.ErrNotExist Jul 18, 2023
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@raghavendra-talur
Copy link
Contributor Author

This fix is based on the suggestion provided in a previous PR that attempted to fix this problem.

@@ -301,6 +301,10 @@ func profileDirs(miniHome ...string) (dirs []string, err error) {
}
pRootDir := filepath.Join(miniPath, "profiles")
items, err := os.ReadDir(pRootDir)
if err == os.IsNotExist(err) {
return dirs, &ErrNotExist{fmt.Sprintf("profiles dir %s does not exist", pRootDir)}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way to handle this is to return here empty list of directories and no error since this is an expected condition before any profile was created, or if a user deleted the profiles directory.

However if we report an error, we should do the check correctly - it seems that you are adding here a wrong check:

if err == os.IsNotExist(err)

And you fix it in the next commit? Did you forget to squash the next commit into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I squashed the next commit. The comparison is good now.

About the error return; I am hoping that that error case is handled specially as it is a ERRNOTEXIST and I am leaving that handling to the caller.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 18, 2023
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2023
@raghavendra-talur
Copy link
Contributor Author

@medyagh @sharifelgamal Could you please take a look at this PR in your next review cycle?

Any feedback or suggestions are most welcome.

@raghavendra-talur
Copy link
Contributor Author

@medyagh Just checking if you have any feedback for this PR. I can close this if you think it is not worth fixing.

nirs added a commit to nirs/ramen that referenced this pull request Nov 15, 2023
If `minikube profile list` is called before `minikube start` it fails
because `$MINIKUBE_HOME/.minikube/profiles` does not exist[1]. I posted
a fix[1] on Jan 5 2023 but it was not accepted. Talur posted another
fix[2] on July 18 2023 but it is still waiting for review.

Fixing it in drenv until minikube maintainers find time to handle this.

[1] kubernetes/minikube#15593
[2] kubernetes/minikube#15594
[3] kubernetes/minikube#16900

Signed-off-by: Nir Soffer <[email protected]>
nirs added a commit to nirs/ramen that referenced this pull request Nov 15, 2023
If `minikube profile list` is called before `minikube start` it fails
because `$MINIKUBE_HOME/.minikube/profiles` does not exist[1]. I posted
a fix[1] on Jan 5 2023 but it was not accepted. Talur posted another
fix[2] on July 18 2023 but it is still waiting for review.

Fixing it in drenv until minikube maintainers find time to handle this.

[1] kubernetes/minikube#15593
[2] kubernetes/minikube#15594
[3] kubernetes/minikube#16900

Signed-off-by: Nir Soffer <[email protected]>
ShyamsundarR pushed a commit to RamenDR/ramen that referenced this pull request Dec 11, 2023
If `minikube profile list` is called before `minikube start` it fails
because `$MINIKUBE_HOME/.minikube/profiles` does not exist[1]. I posted
a fix[1] on Jan 5 2023 but it was not accepted. Talur posted another
fix[2] on July 18 2023 but it is still waiting for review.

Fixing it in drenv until minikube maintainers find time to handle this.

[1] kubernetes/minikube#15593
[2] kubernetes/minikube#15594
[3] kubernetes/minikube#16900

Signed-off-by: Nir Soffer <[email protected]>
ShyamsundarR pushed a commit to red-hat-storage/ramen that referenced this pull request Dec 13, 2023
If `minikube profile list` is called before `minikube start` it fails
because `$MINIKUBE_HOME/.minikube/profiles` does not exist[1]. I posted
a fix[1] on Jan 5 2023 but it was not accepted. Talur posted another
fix[2] on July 18 2023 but it is still waiting for review.

Fixing it in drenv until minikube maintainers find time to handle this.

[1] kubernetes/minikube#15593
[2] kubernetes/minikube#15594
[3] kubernetes/minikube#16900

Signed-off-by: Nir Soffer <[email protected]>
@nirs
Copy link
Contributor

nirs commented Jan 19, 2024

Any chance to get a review of for this tiny fix?

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all 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:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

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

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2024
@nirs
Copy link
Contributor

nirs commented Apr 18, 2024

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 18, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all 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:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

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

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 17, 2024
@medyagh
Copy link
Member

medyagh commented Jul 25, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 25, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: raghavendra-talur
Once this PR has been reviewed and has the lgtm label, please assign medyagh for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Member

@medyagh medyagh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @nirs and @raghavendra-talur for this PR and the review and sorry this PR got lost in the long list of PRs, will run test on it

@minikube-pr-bot
Copy link

kvm2 driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16900) |
+----------------+----------+---------------------+
| minikube start | 49.7s    | 50.4s               |
| enable ingress | 24.1s    | 25.0s               |
+----------------+----------+---------------------+

Times for minikube start: 49.2s 49.5s 49.1s 51.7s 49.1s
Times for minikube (PR 16900) start: 49.6s 53.5s 49.0s 48.5s 51.4s

Times for minikube ingress: 23.5s 24.4s 23.9s 24.4s 24.4s
Times for minikube (PR 16900) ingress: 25.9s 23.4s 23.9s 27.9s 23.9s

docker driver with docker runtime

+----------------+----------+---------------------+
|    COMMAND     | MINIKUBE | MINIKUBE (PR 16900) |
+----------------+----------+---------------------+
| minikube start | 22.2s    | 22.6s               |
| enable ingress | 21.8s    | 22.1s               |
+----------------+----------+---------------------+

Times for minikube start: 21.0s 20.8s 24.0s 23.8s 21.1s
Times for minikube (PR 16900) start: 23.7s 23.4s 21.9s 23.3s 20.9s

Times for minikube ingress: 21.2s 21.8s 21.7s 22.7s 21.7s
Times for minikube (PR 16900) ingress: 22.7s 21.2s 22.7s 22.7s 21.3s

docker driver with containerd runtime

+-------------------+----------+---------------------+
|      COMMAND      | MINIKUBE | MINIKUBE (PR 16900) |
+-------------------+----------+---------------------+
| minikube start    | 22.4s    | 21.5s               |
| ⚠️  enable ingress | 35.3s    | 41.6s ⚠️             |
+-------------------+----------+---------------------+

Times for minikube start: 22.5s 20.6s 23.3s 22.9s 22.5s
Times for minikube (PR 16900) start: 22.5s 23.9s 20.1s 20.8s 20.3s

Times for minikube ingress: 32.2s 31.7s 47.2s 32.2s 33.2s
Times for minikube (PR 16900) ingress: 32.2s 47.2s 48.2s 48.2s 32.2s

@minikube-pr-bot
Copy link

Here are the number of top 10 failed tests in each environments with lowest flake rate.

Environment Test Name Flake Rate
KVM_Linux (1 failed) TestForceSystemdEnv(gopogh) 0.60% (chart)
Docker_Linux_containerd_arm64 (2 failed) TestAddons/serial/Volcano(gopogh) Unknown
Docker_Linux_containerd_arm64 (2 failed) TestStartStop/group/old-k8s-version/serial/SecondStart(gopogh) 47.90% (chart)
Docker_Linux_docker_arm64 (6 failed) TestFunctional/serial/InvalidService(gopogh) 0.00% (chart)
Docker_Linux_docker_arm64 (6 failed) TestFunctional/parallel/NodeLabels(gopogh) 0.00% (chart)
Docker_Linux_docker_arm64 (6 failed) TestFunctional/parallel/DockerEnv/bash(gopogh) 0.00% (chart)
Docker_Linux_docker_arm64 (6 failed) TestFunctional/parallel/TunnelCmd/serial/RunSecondTunnel(gopogh) 0.00% (chart)
Docker_Linux_docker_arm64 (6 failed) TestFunctional/parallel/TunnelCmd/serial/WaitService/Setup(gopogh) 0.00% (chart)
Docker_Linux_docker_arm64 (6 failed) TestFunctional/parallel/TunnelCmd/serial/AccessDirect(gopogh) 0.00% (chart)

Besides the following environments also have failed tests:

To see the flake rates of all tests by environment, click here.

@medyagh
Copy link
Member

medyagh commented Jul 26, 2024

thank you for this PR !

@medyagh medyagh merged commit 6a417d5 into kubernetes:master Jul 26, 2024
28 of 40 checks passed
@medyagh medyagh changed the title config: transform os.ENOENT error into config.ErrNotExist profile list --output json handle empty config folder Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

minikube profile list --output json fails with ENONT after fresh install
7 participants