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

Fixes difficult to understand error message when CSV is missing APIVersion #2673

Merged
merged 1 commit into from
May 4, 2022
Merged

Fixes difficult to understand error message when CSV is missing APIVersion #2673

merged 1 commit into from
May 4, 2022

Conversation

nsapse
Copy link
Contributor

@nsapse nsapse commented Feb 23, 2022 •
edited by anik120
Loading

Description of the change:

Modifies CSV unbundle process so that after unbundling APIVersion or Kind are missing the installPlan is failed and the user receives a helpful and descriptive failure message letting them know that data was missing.

Motivation for the change:

Per BZ#1982737 currently when a CSV is missing APIVersion the associated installPlan fails with a cryptic message involving failure to create a service account. I initially tried to solve this by inferring the APIVersion if left blank per PR#2609 however the conversation following the PR made it clear that approach was not an acceptable fix.

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky
  • Tests that remove the [FLAKE] tag are no longer flaky

@openshift-ci openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 23, 2022
@openshift-ci
Copy link

openshift-ci bot commented Feb 23, 2022

Hi @nsapse. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@nsapse nsapse mentioned this pull request Feb 23, 2022
Closed
5 tasks
@nsapse nsapse changed the title Bug 1982737 Fixes difficult to understand error message when CSV is missing APIVersion Feb 23, 2022
@nsapse nsapse changed the title Fixes difficult to understand error message when CSV is missing APIVersion WIP: Fixes difficult to understand error message when CSV is missing APIVersion Feb 23, 2022
@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. label Feb 23, 2022
@tylerslaton
Copy link
Contributor

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 24, 2022
@dinhxuanvu
Copy link
Member

Just a friendly reminder here is that you need to identify if an e2e test case is needed. The unit test is preferred if e2e is unnecessary.

@nsapse nsapse force-pushed the Bug_1982737_2 branch 6 times, most recently from 036040f to 6777815 Compare March 22, 2022 17:26
@anik120 anik120 changed the title WIP: Fixes difficult to understand error message when CSV is missing APIVersion Fixes difficult to understand error message when CSV is missing APIVersion Mar 22, 2022
@anik120 anik120 removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2022
@nsapse nsapse force-pushed the Bug_1982737_2 branch 2 times, most recently from f29005f to a64cf99 Compare March 22, 2022 18:40
exdx
exdx reviewed Mar 23, 2022
View reviewed changes
Copy link
Member

@exdx exdx left a comment

Choose a reason for hiding this comment

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

Nice job -- I just had a thought on the overall purpose of the implementation. Code-wise everything looks good.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2022
@nsapse nsapse force-pushed the Bug_1982737_2 branch 5 times, most recently from d791f41 to 5d9fc52 Compare April 29, 2022 19:46
exdx
exdx reviewed May 2, 2022
View reviewed changes
Copy link
Member

@exdx exdx left a comment

Choose a reason for hiding this comment

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

small nit: the commit message should be updated to reflect the new fatal error type.

After that, this LGTM

@nsapse
Copy link
Contributor Author

nsapse commented May 2, 2022

Realizing that the test currently references a quay repo in my own repo rather than the olmtest repo which I don't have write access for. That seems like a bad idea from a link rot perspective. What's the most efficient way for me to push my image to the olmtest repo?

@nsapse nsapse force-pushed the Bug_1982737_2 branch 2 times, most recently from 2fab6f4 to fe54a3c Compare May 2, 2022 18:53
@exdx
Copy link
Member

exdx commented May 2, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 2, 2022
@nsapse
FIX: Bug 1982737 - Make malformed CSV fail nicely
7bd349e 
- Modified bundle unpacking to fail installPlan with a nice message if
after unbundling csv manifests APIVersion or Kind are blank.

- Adds FatalError type to return to checked when syncing install plans
 to see if the error causing issues is fatal or should cause the
 IP to be transitioned to failed state.

- Makes installPlan fail with a nicer more user friendly message than
the current difficult to understand message regarding serviceAccounts.

- Adds tests using internal magic-catalog library to  confirm that unpacking
fails when APIVersion is blank.

Signed-off-by: Noah Sapse <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 3, 2022
@tylerslaton
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2022
@perdasilva
Copy link
Collaborator

noice! You're a legend, thank you @nsapse

@perdasilva
Copy link
Collaborator

/approve

@openshift-ci
Copy link

openshift-ci bot commented May 4, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: exdx, nsapse, perdasilva

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. label May 4, 2022
@openshift-merge-robot openshift-merge-robot merged commit 3002cf7 into operator-framework:master May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perdasilva perdasilva perdasilva left review comments

@benluddy benluddy benluddy left review comments

@anik120 anik120 anik120 left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu left review comments

@timflannagan timflannagan timflannagan left review comments

@tylerslaton tylerslaton tylerslaton left review comments

@exdx exdx exdx approved these changes

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@exdx exdx

@tylerslaton tylerslaton

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@nsapse @tylerslaton @dinhxuanvu @exdx @timflannagan @perdasilva @benluddy @anik120 @openshift-merge-robot