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

porch CLI: error instead of panic when the upstreamLock is not parseable #3938

Merged
merged 2 commits into from
May 1, 2023

Conversation

natasha41575
Copy link
Contributor

@natasha41575 natasha41575 commented Apr 28, 2023

Throws an error instead of panicking in cases like #3933.

Sample output:

$ kpt alpha rpkg update --discover=downstream
Error: could not parse upstreamLock in Kptfile of package "blueprints-8c50f85abaa5d22110b7c826bcb77f8f33d39dbb": malformed upstreamLock.Git.Ref "v2"

When using porch CLI, the upstreamLock.Git.Ref should be in the format of either drafts/pkgname/workspace or pkgname/version. So we theoretically shouldn't run into any cases where there is no / in upstreamLock.Git.Ref unless someone makes a change through something other than porch CLI. In these cases, we should throw an error that we are unable to parse the upstreamLock rather than panicking.

@natasha41575 natasha41575 requested a review from a team as a code owner April 28, 2023 01:30
@natasha41575 natasha41575 changed the title porch: fix panic when the upstreamLock is not parseable porch: error instead of panic when the upstreamLock is not parseable Apr 28, 2023
@natasha41575 natasha41575 changed the title porch: error instead of panic when the upstreamLock is not parseable porch CLI: error instead of panic when the upstreamLock is not parseable Apr 28, 2023
Copy link
Contributor

@mortent mortent left a comment

Choose a reason for hiding this comment

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

Maybe we can add a test here, but otherwise looks good.

@natasha41575 natasha41575 force-pushed the porch/updatePanic branch 2 times, most recently from 7b66d73 to 27e7459 Compare May 1, 2023 17:37
@natasha41575
Copy link
Contributor Author

Maybe we can add a test here, but otherwise looks good.

Added, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants