-
Notifications
You must be signed in to change notification settings - Fork 66
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
reordering submit flow to create test results as first action #1151
reordering submit flow to create test results as first action #1151
Conversation
from change #1151: |
/test 4.12-e2e |
internal/pyxis/submit.go
Outdated
// Create the test results, so we can fail fast if version check throws error. | ||
testResults := certInput.TestResults | ||
testResults, err = p.createOrUpdateTestResults(ctx, testResults, http.MethodPost) | ||
if err != nil { | ||
return nil, fmt.Errorf("could not create test results: %v", err) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is here to fail fast, but it would seem that if there are other failures (e.g. if certImage.Repositories == 0
a few lines down), wouldn't we have created a TestResult for this account before we've asserted whether their content is valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, that should probably be first, since it was lost in the comments, I'll move it to be first thing.
e85ac24
to
f4459c3
Compare
from change #1151: |
f4459c3
to
b6eedb7
Compare
from change #1151: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new logic looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about the methods.
…inking to an imageID later in the flow to be able to fail fast and and not have incomplete results attached to an image Signed-off-by: Adam D. Cornett <[email protected]>
b6eedb7
to
7dfeeb0
Compare
from change #1151: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acornett21, Allda, bcrochet, komish 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 |
createTestResults
was modified to account for bothPATCH/POST
, since there would be less duplicate code, if others are happier with two distinct methods, I'm happy to refactor that way as well.