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 list" #6043

Merged

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Aug 22, 2022

What type of PR is this:

/kind feature

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #5991

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Aug 22, 2022
@netlify
Copy link

netlify bot commented Aug 22, 2022

Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 7315268
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/630cdd663fe17b00088606aa
😎 Deploy Preview https://deploy-preview-6043--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 requested review from dharmit and valaparthvi August 22, 2022 11:48
@odo-robot
Copy link

odo-robot bot commented Aug 22, 2022

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

@odo-robot
Copy link

odo-robot bot commented Aug 22, 2022

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

@odo-robot
Copy link

odo-robot bot commented Aug 22, 2022

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

@odo-robot
Copy link

odo-robot bot commented Aug 22, 2022

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

@odo-robot
Copy link

odo-robot bot commented Aug 22, 2022

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

@feloy feloy force-pushed the feature-5991/list-all branch from df200e6 to de50759 Compare August 23, 2022 09:48
@feloy feloy changed the title [WIP] Move "odo list" to "odo list component" Move "odo list" to "odo list component" Aug 23, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Aug 23, 2022
@feloy feloy changed the title Move "odo list" to "odo list component" Implement "odo list" Aug 23, 2022
@feloy feloy force-pushed the feature-5991/list-all branch from de50759 to f03bcbe Compare August 23, 2022 10:19
@feloy feloy closed this Aug 23, 2022
@feloy feloy reopened this Aug 23, 2022
Copy link
Contributor

@valaparthvi valaparthvi left a comment

Choose a reason for hiding this comment

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

The code lgtm apart from the one question.

var localComponent api.ComponentAbstract
if devObj.Data != nil {
localComponent = api.ComponentAbstract{
Name: devObj.Data.GetMetadata().Name,
ManagedBy: "",
RunningIn: []api.RunningMode{},
Type: GetComponentTypeFromDevfileMetadata(devObj.Data.GetMetadata()),
}
}

componentInDevfile := ""
if localComponent.Name != "" {
if !Contains(localComponent, devfileComponents) {
devfileComponents = append(devfileComponents, localComponent)
}
componentInDevfile = localComponent.Name
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to print the local component/binding if --namespace is passed? I find it a little confusing to display the binding/component running in some other namespace to be displayed when I explicitly ask for some other namespace.

➜  102 odo list binding --namespace operators
 ✓  Listing ServiceBindings from the namespace "operators" [3ms]
 NAME                            APPLICATION                     SERVICES                                                 RUNNING IN 
 * my-nodejs-app-cluster-sample  my-nodejs-app-app (Deployment)  cluster-sample (Cluster.postgresql.k8s.enterprisedb.io)  None       
➜  102 odo list binding                      
 ✓  Listing ServiceBindings from the namespace "default" [9ms]
 NAME                            APPLICATION                     SERVICES                                                 RUNNING IN 
 * my-nodejs-app-cluster-sample  my-nodejs-app-app (Deployment)  cluster-sample (Cluster.postgresql.k8s.enterprisedb.io)  None       

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kadel what's your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we decide to, I would prefer to make this change as part as another PR

Copy link
Member

Choose a reason for hiding this comment

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

I agree it seems a bit confusing to display the local component/binding when requesting some other namespace (especially since the first line printed is Listing {ServiceBindings,resources} from the namespace "$ns").
But as discussed, it is okay to change this in a separate PR.

@feloy feloy requested review from rm3l and valaparthvi and removed request for dharmit August 29, 2022 07:20
Copy link
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

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

LGTM feature-wise.

/approve

Added a few comments related to documentation.

docs/website/docs/command-reference/list-component.md Outdated Show resolved Hide resolved
docs/website/docs/command-reference/list-component.md Outdated Show resolved Hide resolved
@openshift-ci
Copy link

openshift-ci bot commented Aug 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l

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 Aug 29, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 29, 2022
@feloy feloy force-pushed the feature-5991/list-all branch from 2e365c6 to 7315268 Compare August 29, 2022 15:38
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 29, 2022
@feloy
Copy link
Contributor Author

feloy commented Aug 29, 2022

LGTM feature-wise.

/approve

Added a few comments related to documentation.

Thanks for your review @rm3l

I approved the docs changes and rebased on master to fix the conflicts.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 29, 2022
@feloy feloy closed this Aug 29, 2022
@feloy feloy reopened this Aug 29, 2022
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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
10.5% 10.5% Duplication

@feloy
Copy link
Contributor Author

feloy commented Aug 29, 2022

/override ci/prow/v4.10-integration-e2e
Tests pass on IBM Cloud

/override SonarCloud Code Analysis

@openshift-ci
Copy link

openshift-ci bot commented Aug 29, 2022

@feloy: Overrode contexts on behalf of feloy: SonarCloud Code Analysis, ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e
Tests pass on IBM Cloud

/override SonarCloud Code Analysis

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.

@openshift-merge-robot openshift-merge-robot merged commit 64ee3db into redhat-developer:main Aug 29, 2022
cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
* Move "odo list" to "odo list component"

* Refactor odo list component

* Add --namespace flag to "odo list binding"

* odo list implementation

* Doc

* Apply suggestions from code review

Co-authored-by: Armel Soro <[email protected]>

Co-authored-by: Armel Soro <[email protected]>
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.

odo list should be combined output of odo list component and odo list binding
4 participants