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.

Sign up for GitHub

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

Jump to bottom

List all services in JSON output #5025

Merged
merged 2 commits into from
Sep 3, 2021
Merged

List all services in JSON output #5025

merged 2 commits into from
Sep 3, 2021

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Aug 27, 2021

/kind bug

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #4992

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 27, 2021
@feloy feloy requested review from valaparthvi and removed request for mik-dass August 27, 2021 13:33
@valaparthvi
Copy link
Member

This is somewhat irrelevant to this PR but, do we need component to create a service? If connecting a service and component is only possible via linking, then what is the point in creating the service via a component first? Can't service be independent of a component? (I vaguely remember that we have had these discussions in the past but I don't remember what was concluded).

valaparthvi
valaparthvi suggested changes Aug 27, 2021
View reviewed changes
Copy link
Member

@valaparthvi valaparthvi left a comment •
edited

Choose a reason for hiding this comment

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

The PR looks good to me apart from a few things. Functionality looks good as well; tested with pushed/not-pushed/locally-deleted/not-managed-by-odo services.

Should we add an integration test for services not managed by odo?

pkg/odo/cli/service/list_operator.go Outdated
if log.IsJSON() {
if len(servicesItems.Items) == 0 {
if len(failedListingCR) > 0 {
fmt.Printf("Failed to fetch services for operator(s): %q\n\n", strings.Join(failedListingCR, ", "))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this not a fmt.Errorf statement?

pkg/odo/cli/service/list_operator_test.go Outdated Show resolved Hide resolved
pkg/odo/cli/service/list_operator_test.go Outdated Show resolved Hide resolved
pkg/odo/cli/service/list_operator.go Outdated Show resolved Hide resolved
pkg/odo/cli/service/list_operator.go Show resolved Hide resolved
pkg/odo/cli/service/list_operator.go Outdated Show resolved Hide resolved
pkg/service/service.go Show resolved Hide resolved
@feloy feloy force-pushed the bugfix-4992/list-non-pushed-services branch from 43d315f to 1b4a029 Compare August 30, 2021 13:58
valaparthvi
valaparthvi suggested changes Aug 30, 2021
View reviewed changes
pkg/service/service_test.go Show resolved Hide resolved
pkg/service/service_test.go Show resolved Hide resolved
@feloy feloy force-pushed the bugfix-4992/list-non-pushed-services branch from 1b4a029 to d87078e Compare August 30, 2021 15:06
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 31, 2021
dharmit
dharmit reviewed Sep 1, 2021
View reviewed changes
Copy link
Member

@dharmit dharmit left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link

openshift-ci bot commented Sep 1, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

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 Sep 1, 2021
@dharmit
Copy link
Member

dharmit commented Sep 1, 2021

This is somewhat irrelevant to this PR but, do we need component to create a service? If connecting a service and component is only possible via linking, then what is the point in creating the service via a component first? Can't service be independent of a component? (I vaguely remember that we have had these discussions in the past but I don't remember what was concluded).

This has come up more than a few times during various conversations. AFAIK, @kadel is looking into this aspect along with @deboer-tim.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@feloy
Copy link
Contributor Author

feloy commented Sep 2, 2021

/retest-required

CI fix has been merged (#5028)

@feloy feloy force-pushed the bugfix-4992/list-non-pushed-services branch from d87078e to fbdf5a6 Compare September 2, 2021 11:04
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Sep 2, 2021
@sonarcloud
Copy link

sonarcloud bot commented Sep 2, 2021

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 1 Code Smell

No Coverage information No Coverage information
0.4% 0.4% Duplication

@feloy
Copy link
Contributor Author

feloy commented Sep 2, 2021

/test psi-unit-test-windows

[ssh:Windows 10] Pushing files...
[ssh:Windows 10] panic: test timed out after 10m0s
[ssh:Windows 10] 
[ssh:Windows 10] goroutine 16 [running]:
[ssh:Windows 10] testing.(*M).startAlarm.func1()
[ssh:Windows 10] 	c:/go/src/testing/testing.go:1628 +0xe6
[ssh:Windows 10] created by time.goFunc
skipped 128 lines unfold_more
[ssh:Windows 10] 	c:/go/src/sync/mutex.go:81
[ssh:Windows 10] github.com/openshift/odo/pkg/watch.WatchAndPush.func1(0xc00012ba20, 0xc00037f760, 0xc0002cade0, 0xc0000c9800, 0xc000447c80, 0xc000447c40, 0xc00037f768, 0xc000447c60)
[ssh:Windows 10] 	C:/Users/Admin/amqp_ci_rcv_odo-windows-unit-pr-build_PR_5025/PKFgH/repo/pkg/watch/watch.go:198 +0xf05
[ssh:Windows 10] created by github.com/openshift/odo/pkg/watch.WatchAndPush
[ssh:Windows 10] 	C:/Users/Admin/amqp_ci_rcv_odo-windows-unit-pr-build_PR_5025/PKFgH/repo/pkg/watch/watch.go:187 +0x3da
[ssh:Windows 10] FAIL	github.com/openshift/odo/pkg/watch	600.641s
[ssh:Windows 10] FAIL

@feloy
Copy link
Contributor Author

feloy commented Sep 2, 2021

/test psi-kubernetes-integration-e2e

[ssh:Fedora-32-minikube]         mkdir /tmp/475500516: no space left on device

@valaparthvi
Copy link
Member

/lgtm

Adding again because it was removed due to rebase.

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

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 354a9d0 into redhat-developer:main Sep 3, 2021
@kadel kadel mentioned this pull request Sep 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dharmit dharmit dharmit left review comments

@valaparthvi valaparthvi valaparthvi approved these changes

Assignees

@valaparthvi valaparthvi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to retrieve a non pushed service
5 participants
@feloy @valaparthvi @dharmit @openshift-bot @openshift-merge-robot