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

remove abrubt os.Exit calls #5087

Conversation

anandrkskd
Copy link
Contributor

@anandrkskd anandrkskd commented Sep 23, 2021

Signed-off-by: anandrkskd [email protected]

What type of PR is this?
/kind bug
What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #4999

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 Sep 23, 2021
@openshift-ci openshift-ci bot requested review from feloy and valaparthvi September 23, 2021 16:15
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.

@anandrkskd
Copy link
Contributor Author

/test v4.8-integration-e2e

@valaparthvi
Copy link
Contributor

valaparthvi commented Sep 28, 2021

All the context related functions in context.go should return *Context, and error and this error should be returned in order to fix this telemetry error.
The following places should return an error to be bubbled up -

  1. internalCxt.resolveProject - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/resolve.go#L31
  2. genericclioptions.GetContextFlagValue - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/flag.go#L34 and https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/flag.go#L38
  3. internalCxt.resolveNamespace - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/resolve.go#L70
    and checkProjectCreateOrDeleteOnlyOnInvalidNamespaceNoFmt - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/util.go#L47
  4. genericclioptions.ApplyIgnore - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/util.go#L71
  5. CheckMachineReadableOutputCommand - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/runnable.go#L191

Other simple and might require more work places that need fixing -

  1. IsSecure - https://github.com/openshift/odo/blob/main/pkg/odo/cli/registry/util/util.go#L23
  2. ComponentAllowingEmpty - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/context.go#L373 & https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/context.go#L381
  3. checkComponentExistsOrFail - https://github.com/openshift/odo/blob/main/pkg/odo/genericclioptions/util.go#L61

Other not so straightforward places that need fixing -

  1. DevfilePush - https://github.com/openshift/odo/blob/main/pkg/odo/cli/component/devfile.go#L51
  2. DisplayLog - https://github.com/openshift/odo/blob/main/pkg/util/util.go#L1281
  3. HandleError - https://github.com/openshift/odo/blob/main/pkg/odo/cli/ui/common.go#L12; I am not sure whether we want to fix this or keep it as is.
  4. CopyFile - https://github.com/openshift/odo/blob/main/pkg/sync/sync.go#L44

@valaparthvi
Copy link
Contributor

valaparthvi commented Sep 28, 2021

Other not so straightforward places that need fixing -

  1. DevfilePush - https://github.com/openshift/odo/blob/main/pkg/odo/cli/component/devfile.go#L51
  2. DisplayLog - https://github.com/openshift/odo/blob/main/pkg/util/util.go#L1281
  3. HandleError - https://github.com/openshift/odo/blob/main/pkg/odo/cli/ui/common.go#L12; I am not sure whether we want to fix this or keep it as is.

Hey @anandrkskd, @feloy suggested that we can make the following points an issue for Hacktoberfest and I think it's a good suggestion, these issues wouldn't be completely straightforward but still good enough. If you are okay with it, I will go ahead and create the issues.

@anandrkskd
Copy link
Contributor Author

anandrkskd commented Sep 28, 2021

Hey @anandrkskd, @feloy suggested that we can make the following points an issue for Hacktoberfest and I think it's a good suggestion, these issues wouldn't be completely straightforward but still good enough. If you are okay with it, I will go ahead and create the issues.

Yeah sure @valaparthvi, make sense

@anandrkskd anandrkskd force-pushed the dev/telemetery-os-Exit-call branch from 78d58da to 3ad14ec Compare September 29, 2021 10:41
@anandrkskd
Copy link
Contributor Author

Test failed due to timeout error. Retesting
/test psi-kubernetes-integration-e2e

@anandrkskd
Copy link
Contributor Author

@valaparthvi can you review this PR, I have made all the changes.
Thanks.

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.

pkg/devfile/adapters/kubernetes/component/adapter.go Outdated Show resolved Hide resolved
pkg/odo/cli/component/common_push.go Outdated Show resolved Hide resolved
pkg/odo/genericclioptions/util.go Outdated Show resolved Hide resolved
pkg/odo/genericclioptions/util.go Outdated Show resolved Hide resolved
pkg/odo/genericclioptions/context.go Outdated Show resolved Hide resolved
pkg/odo/genericclioptions/context.go Outdated Show resolved Hide resolved
pkg/odo/cli/registry/util/util_test.go Outdated Show resolved Hide resolved
pkg/odo/cli/registry/util/util.go Outdated Show resolved Hide resolved
pkg/odo/cli/registry/list.go Outdated Show resolved Hide resolved
pkg/odo/cli/component/component.go Outdated Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Oct 1, 2021

✔️ Deploy Preview for odo-docusaurus-preview ready!

🔨 Explore the source changes: f673f55

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/6156e37fd56b9f0007f8289d

😎 Browse the preview: https://deploy-preview-5087--odo-docusaurus-preview.netlify.app

@anandrkskd
Copy link
Contributor Author

/test psi-kubernetes-integration-e2e

2 similar comments
@anandrkskd
Copy link
Contributor Author

/test psi-kubernetes-integration-e2e

@anandrkskd
Copy link
Contributor Author

/test psi-kubernetes-integration-e2e

@anandrkskd
Copy link
Contributor Author

@valaparthvi can you /lgtm this PR, I have updated all the change requested.

pkg/odo/genericclioptions/context.go Outdated Show resolved Hide resolved
pkg/odo/genericclioptions/util.go Outdated Show resolved Hide resolved
pkg/odo/genericclioptions/util.go Outdated Show resolved Hide resolved
Signed-off-by: anandrkskd <[email protected]>
@openshift-ci
Copy link

openshift-ci bot commented Oct 5, 2021

@anandrkskd: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/psi-k8s-ibmc-integration-e2e f673f55 link false /test psi-k8s-ibmc-integration-e2e

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

pkg/odo/genericclioptions/util.go Outdated Show resolved Hide resolved
pkg/odo/genericclioptions/util.go Outdated Show resolved Hide resolved
Signed-off-by: anandrkskd <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 6, 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 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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

/approve

@openshift-ci
Copy link

openshift-ci bot commented Oct 6, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: valaparthvi

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 Oct 6, 2021
@openshift-merge-robot openshift-merge-robot merged commit a83ce2f into redhat-developer:main Oct 6, 2021
@anandrkskd anandrkskd deleted the dev/telemetery-os-Exit-call branch October 13, 2022 11:12
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/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
Development

Successfully merging this pull request may close these issues.

Telemetry does not catch all errors due to abrupt os.Exit calls
4 participants