-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 build \!remote flags from test phase 2 #8379
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
/lgtm |
I see that someone pressed Re-run. I don't think that's going to help: these look like very real errors: lots in ubuntu and only push in fedora |
@edsantiago I found a race condition on podman-remote network rm --force, that is causing the network remove failures. |
97658ff
to
175b482
Compare
@containers/podman-maintainers PTAL |
return | ||
} | ||
if err := network.RemoveNetwork(config, name); err != nil { | ||
utils.InternalServerError(w, err) | ||
report := reports[0] |
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.
Should do a length check to make sure it is at least len(1) so we don't panic
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.
Fixed
|
||
decoder := r.Context().Value("decoder").(*schema.Decoder) | ||
if err := decoder.Decode(&query, r.URL.Query()); err != nil { | ||
utils.Error(w, "Something went wrong.", http.StatusBadRequest, errors.Wrapf(err, "failed to parse parameters for %s", r.URL.String())) |
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'm not going to block just because of this... but if you need to resubmit for any reason, could there maybe be a better diagnostic than "Something went wrong"?
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.
Fixed, although this message is in all over the code. I think this is what Docker did when there was an unexpected error.
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.
@edsantiago I thoroughly concur and have been dinging those as I encounter them. It makes me grind my teeth every time I spot it.
The --force parameter was not being handled correctly. This is leading to some race conditions in testing failures. Signed-off-by: Daniel J Walsh <[email protected]>
Tentatively LGTM aside from two questionable "Something went wrong" messages. I appreciate the |
Add some more tests, document cases where remote will not work Add FIXMEs for tests that should work on podman-remote but currently do not. Signed-off-by: Daniel J Walsh <[email protected]>
LGTM once @QiWang19 's question is answered/addressed. |
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
Add some more tests, document cases where remote will not work
Add FIXMEs for tests that should work on podman-remote but currently
do not.
Signed-off-by: Daniel J Walsh [email protected]