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

Add missing early returns in compat API #9360

Merged

Conversation

riyad
Copy link
Contributor

@riyad riyad commented Feb 14, 2021

While looking to "fix" #9314 I found a few more missing early returns.
This only fixes the double response part of #9314.

The weird thing is utils.Error() totally ignores the 2nd (i.e. apiMessage) argument which is sometimes used to add context to the error, but doesn't end up in either the logs or the response. This seems to be an oversight since its initial implementation. o.O

Closes #9314

@@ -25,12 +25,14 @@ func ListSecrets(w http.ResponseWriter, r *http.Request) {
}{}

if err := decoder.Decode(&query, r.URL.Query()); err != nil {
utils.Error(w, http.StatusText(http.StatusBadRequest), http.StatusBadRequest,
utils.Error(w, "Something went wrong.", http.StatusBadRequest,
Copy link
Collaborator

@zhangguanzhang zhangguanzhang Feb 14, 2021

Choose a reason for hiding this comment

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

I think should not change this line

Copy link
Contributor Author

@riyad riyad Feb 14, 2021

Choose a reason for hiding this comment

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

The idea was to make it more similar to other endpoints ... I was under the impression that the "Something went wrong.-style is the most common. But trying to prove my point I found quite some counter examples (see below). 🤷
If you still want it I can revert this.

"Something went wrong.-style examples:

http.StatusText(http.StatusBadRequest)-style examples:

totally different style:

@riyad riyad force-pushed the add-missing-early-returns branch from 965c340 to 19fdfab Compare February 14, 2021 20:32
@zhangguanzhang
Copy link
Collaborator

should signed of at the git commit messages

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Changes LGTM

To pass CI, the commit must be signed: git commit -s --amend and repush

@riyad riyad force-pushed the add-missing-early-returns branch from 19fdfab to 67d74e8 Compare February 15, 2021 10:17
@rhatdan
Copy link
Member

rhatdan commented Feb 15, 2021

You also need to add

[NO TESTS NEEDED]

To this commit, otherwise you will need to add tests.

LGTM

@riyad riyad force-pushed the add-missing-early-returns branch 3 times, most recently from 444d999 to fa9429a Compare February 16, 2021 14:16
@rhatdan
Copy link
Member

rhatdan commented Feb 16, 2021

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, riyad

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2021
@mheon
Copy link
Member

mheon commented Feb 16, 2021

/hold
CI doesn't seem to have run
@cevich PTAL

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2021
[NO TESTS NEEDED]

Signed-off-by: Riyad Preukschas <[email protected]>
@riyad riyad force-pushed the add-missing-early-returns branch from fa9429a to 68a8d39 Compare February 16, 2021 22:40
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 16, 2021
@riyad
Copy link
Contributor Author

riyad commented Feb 17, 2021

I'm not sure what new changes the bot has detected. 😅 I just rebased the PR to re-trigger the CI.

@rhatdan
Copy link
Member

rhatdan commented Feb 17, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021
@zhangguanzhang
Copy link
Collaborator

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 17, 2021
@openshift-merge-robot openshift-merge-robot merged commit d55d80a into containers:master Feb 17, 2021
@cevich
Copy link
Member

cevich commented Feb 17, 2021

CI doesn't seem to have run

Hmmm, yeah, I see it's not just Cirrus-CI but nothing ran for fa9429a9474ff8190b7a8686f43b5ba0ce9392c2 Most likely this was a Github problem and not a Cirrus-CI problem. I'll keep on the lookout for this happening again.

@riyad riyad deleted the add-missing-early-returns branch February 17, 2021 22:24
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APIv2] /secrets endpoint with filter param doesn't filter and returns 400 with two responses
8 participants