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

Implement odo set namespace/project #5744

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented May 16, 2022

What type of PR is this:
/kind feature

What does this PR do / why we need it:
This PR continues the work started by @valaparthvi by implementing the missing odo set namespace/project command.

Which issue(s) this PR fixes:
Relates to #5525

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:
Note: as depicted in the acceptance criteria, if odo set namespace is executed inside a component directory, it should show a warning, that this won't update the namespace of the existing component.

❯ odo help set namespace
Set the current active namespace.

 If executed inside a component directory, this command will not update the namespace of the existing component.

Usage:
  odo set namespace [flags]

Aliases:
  namespace, project

Examples:
  # Set the specified namespace as the current active namespace in the config
  odo set namespace my-namespace

Flags:
  -h, --help   Help for namespace

Additional Flags:
      --kubeconfig string    Paths to a kubeconfig. Only required if out-of-cluster.
  -v, --v Level              Number for the log level verbosity. Level varies from 0 to 9 (default 0).
      --vmodule moduleSpec   Comma-separated list of pattern=N settings for file-filtered logging

@rm3l rm3l requested review from feloy and valaparthvi May 16, 2022 09:09
@netlify
Copy link

netlify bot commented May 16, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit c8f21e9
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/628258bb8bdd2a0008a4d770
😎 Deploy Preview https://deploy-preview-5744--odo-docusaurus-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@openshift-ci openshift-ci bot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label May 16, 2022
@openshift-ci openshift-ci bot requested review from cdrage and rnapoles-rh May 16, 2022 09:09
@rm3l rm3l mentioned this pull request May 16, 2022
8 tasks
@odo-robot
Copy link

odo-robot bot commented May 16, 2022

Unit Tests on commit 91baca2 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented May 16, 2022

Windows Tests (OCP) on commit 91baca2 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented May 16, 2022

Kubernetes Tests on commit 91baca2 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented May 16, 2022

OpenShift Tests on commit 91baca2 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented May 16, 2022

Validate Tests on commit 91baca2 finished successfully.
View logs: TXT HTML

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 16, 2022
checkNsDeletionFunc(true)
It(fmt.Sprintf("should successfully delete the %s synchronously with --wait", commandName), func() {
checkNsDeletionFunc([]string{"--wait"}, func() {
Expect(commonVar.CliRunner.GetAllNamespaceProjects()).ShouldNot(ContainElement(namespace))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you try to use kubectl get namespace <name> instead of the list?

We are experiencing flaky errors where the namespace is in the list. Is it due to some cache or some delay in the list update?

Copy link
Member Author

@rm3l rm3l May 16, 2022

Choose a reason for hiding this comment

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

Ok. I remember I tried to use one of the existing methods (one of helper.GetNamespaceProject or helper.CheckNamespaceProjectExists) but couldn't because they expect that the "kubectl get namespace <name>" command passes. So I couldn't use it to check that a namespace no longer exists. That's why I tried with the list.
But let me fix that.

Copy link
Member Author

@rm3l rm3l May 16, 2022

Choose a reason for hiding this comment

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

We are experiencing flaky errors where the namespace is in the list. Is it due to some cache or some delay in the list update?

I quickly tested on a cluster-bot cluster, and from what I understand from the API Server response headers, kubectl could indeed cache responses, but the response must be validated with the origin server before each reuse. So to me, on the kubectl client side, responses are potentially cached but always checked for content updates.

❯ kubectl get ns --v=8
Alias tip: k get ns --v=8
I0516 13:41:26.906356  420945 loader.go:372] Config loaded from file:  /home/asoro/work/tmp/cluster-bot/config
I0516 13:41:26.908921  420945 round_trippers.go:463] GET https://127.0.0.1:43051/api/v1/namespaces?limit=500
I0516 13:41:26.908931  420945 round_trippers.go:469] Request Headers:
I0516 13:41:26.908943  420945 round_trippers.go:473]     Accept: application/json;as=Table;v=v1;g=meta.k8s.io,application/json;as=Table;v=v1beta1;g=meta.k8s.io,application/json
I0516 13:41:26.908951  420945 round_trippers.go:473]     User-Agent: kubectl/v1.24.0 (linux/amd64) kubernetes/4ce5a89
I0516 13:41:26.914511  420945 round_trippers.go:574] Response Status: 200 OK in 5 milliseconds
I0516 13:41:26.914524  420945 round_trippers.go:577] Response Headers:
I0516 13:41:26.914534  420945 round_trippers.go:580]     Audit-Id: 8dcfdfbf-47b7-40aa-a89e-e4547f653c8d
I0516 13:41:26.914544  420945 round_trippers.go:580]     Cache-Control: no-cache, private
I0516 13:41:26.914554  420945 round_trippers.go:580]     Content-Type: application/json
I0516 13:41:26.914561  420945 round_trippers.go:580]     X-Kubernetes-Pf-Flowschema-Uid: 915fbd93-565d-4fc2-8975-4dae2b4dcbea
I0516 13:41:26.914568  420945 round_trippers.go:580]     X-Kubernetes-Pf-Prioritylevel-Uid: 98f3f965-3d3d-44fc-99d8-d6428ea1ffb3
I0516 13:41:26.914576  420945 round_trippers.go:580]     Date: Mon, 16 May 2022 11:41:26 GMT

I don't know how the --wait flag behaves exactly under the hood, but maybe we are not guaranteed that the namespace is really deleted when the project.Delete method returns?

Anyways, I am trying to rely on the kubectl get namespace <name> command instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another cause would be that, in the sources (kclient/project.go), we are waiting for the deletion of the project, not the namespace

Copy link
Member Author

Choose a reason for hiding this comment

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

But a Project is also a Namespace, no?

Anyways, the latest commit I've pushed no longer uses the list to check for the namespace. I've also rebased the branch to get the other fix related to the Devfile metadata version.
Let's see how it goes now 🤞🏿

@rm3l rm3l force-pushed the 5525-odo-create-delete-list-namespace_set branch from 6afaa5c to c8f21e9 Compare May 16, 2022 13:59
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 16, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rm3l rm3l requested a review from feloy May 16, 2022 15:18
@feloy
Copy link
Contributor

feloy commented May 16, 2022

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label May 16, 2022
@openshift-ci
Copy link

openshift-ci bot commented May 16, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label May 16, 2022
@openshift-merge-robot openshift-merge-robot merged commit de32422 into redhat-developer:main May 16, 2022
@rm3l rm3l deleted the 5525-odo-create-delete-list-namespace_set branch May 17, 2022 07:26
cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
* Implement 'set namespace/project'

* Add doc for 'set namespace/project'

* Add integration tests

* Group tests in cmd_namespace_test.go to make them easier to understand and debug

* Restructure 'set namespace' tests

* Check for namespace/project existence with '{oc,kubectl} get {namespace,project} <name>'

For some reason, checking by listing all namespaces/projects could end up returning an outdated list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants