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

Refactor genericclioptions.Context #5198

Merged
merged 17 commits into from
Nov 17, 2021

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Oct 29, 2021

What type of PR is this?

/kind cleanup

What does this PR do / why we need it:

Which issue(s) this PR fixes:

Fixes #5176

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

@feloy feloy requested review from valaparthvi and dharmit and removed request for dharmit and mohammedzee1000 October 29, 2021 19:58
@feloy
Copy link
Contributor Author

feloy commented Oct 29, 2021

/test unit

@feloy feloy changed the title Refactor genericclioptions.Context [WIP] Refactor genericclioptions.Context Oct 31, 2021
@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 Oct 31, 2021
@netlify
Copy link

netlify bot commented Nov 4, 2021

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

🔨 Explore the source changes: 5e029f7

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

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

@feloy feloy force-pushed the refactor-context branch 2 times, most recently from f5eb35a to 2fbfab3 Compare November 8, 2021 09:23
pkg/odo/genericclioptions/context.go Outdated Show resolved Hide resolved
@feloy feloy changed the title [WIP] Refactor genericclioptions.Context Refactor genericclioptions.Context Nov 9, 2021
@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 Nov 9, 2021
@valaparthvi
Copy link
Contributor

/test psi-kubernetes-integration-e2e
It tends to show old result.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Nov 9, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Nov 9, 2021
Comment on lines 165 to 167
if err != nil {
util.LogErrorAndExit(err, "")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix this function to return the error as well so that we can avoid abrupt exit calls?

Copy link
Contributor Author

@feloy feloy Nov 17, 2021

Choose a reason for hiding this comment

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

This function is called during completion, and I'm not sure if we can modify its signature easily. I would prefer the change is part of another PR concerning completion.

return CreateParameters{cmd: cmd}
}

func (o CreateParameters) CheckRouteAvailability() CreateParameters {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: Can we rename this method to something like RequireRouteAvailability? I understand your reasoning behind naming it CheckRouteAvailability, but I still find it a little non-intuitive. Intuitively I think it would be a function that checks if the route is available to use.

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

No Coverage information No Coverage information
3.7% 3.7% Duplication

@feloy
Copy link
Contributor Author

feloy commented Nov 17, 2021

/test v4.9-integration-e2e

Error: Provider produced inconsistent result after apply

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.

Thank you for this PR :)

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

/approve

@openshift-ci
Copy link

openshift-ci bot commented Nov 17, 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 Nov 17, 2021
@openshift-merge-robot openshift-merge-robot merged commit 4947ec6 into redhat-developer:main Nov 17, 2021
anandrkskd pushed a commit to anandrkskd/odo that referenced this pull request Dec 7, 2021
* Make context.project immutable

* Make context.application immutable

* Make context.outputFlag immutable

* Make context.ComponentContext immutable + dont store command

* Doc + kclient/occlient

* Refactor New*

* More refactoring

* Remove DevfilePath parameter

* genericclioptions.NewCreateParameters

* private consts

* Refactor use of context

* Make CreateParameters exported

* Refactor build-images

* Remove SetComponentcontext

* resolveProjectAndNamespace

* temp fix

* Rename RequireRouteAvailability
@rm3l rm3l added the area/refactoring Issues or PRs related to code refactoring label Jun 16, 2023
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. area/refactoring Issues or PRs related to code refactoring 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.

pkg/odo/genericclioptions usability
4 participants