Skip to content

odo logs: Do not panic when no access to cluster/podman#6561

Merged
openshift-merge-robot merged 4 commits intoredhat-developer:mainfrom
feloy:bugfix-6555/odo-logs-panics
Feb 6, 2023
Merged

odo logs: Do not panic when no access to cluster/podman#6561
openshift-merge-robot merged 4 commits intoredhat-developer:mainfrom
feloy:bugfix-6555/odo-logs-panics

Conversation

@feloy
Copy link
Contributor

@feloy feloy commented Feb 1, 2023

What type of PR is this:

/kind bug
What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #6555

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

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 Feb 1, 2023
@netlify
Copy link

netlify bot commented Feb 1, 2023 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview ready!

Name Link
🔨 Latest commit 15ffeea
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/63dcfb2809ced100086cc910
😎 Deploy Preview https://deploy-preview-6561--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 anandrkskd and rm3l February 1, 2023 17:06
@feloy feloy removed the request for review from anandrkskd February 1, 2023 17:07
@feloy feloy assigned valaparthvi and unassigned valaparthvi Feb 1, 2023
@feloy feloy requested a review from valaparthvi February 1, 2023 17:07
@feloy feloy changed the title odo logs: Do not panic when no access to cluster/podman [wip] odo logs: Do not panic when no access to cluster/podman Feb 1, 2023
@openshift-ci openshift-ci bot added 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 Feb 1, 2023
@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

NoCluster Tests on commit 843e634 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

OpenShift Unauthenticated Tests on commit 843e634 finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

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

@odo-robot
Copy link

odo-robot bot commented Feb 1, 2023 •
edited
Loading

Kubernetes Docs Tests on commit bd0d2ef finished with errors.
View logs: TXT HTML

@feloy feloy force-pushed the bugfix-6555/odo-logs-panics branch from 5f9f9a6 to 63c7321 Compare February 2, 2023 15:29
@feloy feloy changed the title [wip] odo logs: Do not panic when no access to cluster/podman odo logs: Do not panic when no access to cluster/podman Feb 2, 2023
@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 Feb 2, 2023
valaparthvi
valaparthvi suggested changes Feb 3, 2023
View reviewed changes
tests/integration/cmd_logs_test.go Outdated
Comment on lines +79 to +80
When("podman is not installed", func() {
})
Copy link
Contributor

@valaparthvi valaparthvi Feb 3, 2023

Choose a reason for hiding this comment

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

I am assuming this needs to be removed?

feloy
feloy commented Feb 3, 2023
View reviewed changes
@feloy feloy requested a review from valaparthvi February 3, 2023 09:49
valaparthvi
valaparthvi suggested changes Feb 3, 2023
View reviewed changes
pkg/odo/cli/logs/logs.go Outdated
Comment on lines +97 to +101
return errors.New("you need access to a cluster to run this command")
}
case commonflags.PlatformPodman:
if o.clientset.PodmanClient == nil {
return errors.New("you need access to podman to run this command")
Copy link
Contributor

@valaparthvi valaparthvi Feb 3, 2023

Choose a reason for hiding this comment

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

Can you rebase this on top of main and use the new kclient.NewNoConnectionError() function?
Same for podman, podman.NewPodmanNotFoundError().

@feloy feloy force-pushed the bugfix-6555/odo-logs-panics branch from e4e084d to 15ffeea Compare February 3, 2023 12:16
@feloy feloy requested a review from valaparthvi February 3, 2023 12:16
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 3, 2023

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 reviewed Feb 3, 2023
View reviewed changes
rm3l
rm3l reviewed Feb 3, 2023
View reviewed changes
Copy link
Member

@rm3l rm3l left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM after testing it.
@valaparthvi, since you initially reviewed it, please approve this PR if you are ok with the changes.

@valaparthvi
Copy link
Contributor

valaparthvi commented Feb 6, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Feb 6, 2023
@valaparthvi
Copy link
Contributor

valaparthvi commented Feb 6, 2023

/override Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests
Flake:


  user-guides/quickstart/docs-mdx/java/java_odo_dev_output.mdx
  Expected
      <string>:   (
        	"""
        	... // 10 identical lines
        	✓  Added storage m2 to component
        	âš   Pod is Pending
      - 	✓  Pod is Running
        	✓  Syncing files into the container [1s]
        	✓  Building your application in container (command: build) [1s]
        	... // 9 identical lines
        	[Ctrl+c] - Exit and delete resources from the cluster
        	[p] - Manually apply local changes to the application on the cluster
      + 	✓  Pod is Running
        	```
        	"""
        )
      
  to be empty
  In [It] at: /go/odo_1/tests/documentation/user-guides/doc_user_guides_quickstart_test.go:256

@openshift-ci
Copy link

openshift-ci bot commented Feb 6, 2023

@valaparthvi: Overrode contexts on behalf of valaparthvi: Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests

Details

In response to this:

/override Kubernetes-Integration-Tests/Kubernetes-Docs-Integration-Tests
Flake:


 user-guides/quickstart/docs-mdx/java/java_odo_dev_output.mdx
 Expected
     <string>:   (
       	"""
       	... // 10 identical lines
       	✓  Added storage m2 to component
       	âš   Pod is Pending
     - 	✓  Pod is Running
       	✓  Syncing files into the container [1s]
       	✓  Building your application in container (command: build) [1s]
       	... // 9 identical lines
       	[Ctrl+c] - Exit and delete resources from the cluster
       	[p] - Manually apply local changes to the application on the cluster
     + 	✓  Pod is Running
       	```
       	"""
       )
     
 to be empty
 In [It] at: /go/odo_1/tests/documentation/user-guides/doc_user_guides_quickstart_test.go:256

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 704ff2a into redhat-developer:main Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rm3l rm3l rm3l left review comments

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@valaparthvi valaparthvi

Labels

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.

odo logs panics if there is no Kubeconfig

4 participants

@feloy @valaparthvi @rm3l @openshift-merge-robot