Bug 1872557: handle 201 response from upload #168
Conversation
|
Hi @jhjaggars. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
openshift-ci-robot
added
the
needs-ok-to-test
jhjaggars
force-pushed
the
master
branch
2 times, most recently
from
a4b3540 to
4c3dddd
Compare
| counterRequestSend.WithLabelValues(c.metricsName, strconv.Itoa(resp.StatusCode)).Inc() | ||
| } | ||
|
|
||
| if resp.StatusCode >= 400 || resp.StatusCode < 200 { |
There was a problem hiding this comment.
We might want to check and see if we want to call out 3xx, because Go doesn't auto-POST after following redirects, if I recall correctly.
There was a problem hiding this comment.
@jhjaggars Would it be ever neccessary to presume re-POST after 3xx, or can we rely that server will be processing eventual redirections as 2xx ?
From what I read about this it is preferred option.
There was a problem hiding this comment.
What about response.StatusCode / 100 != 2
There was a problem hiding this comment.
re: 3XX, the operator didn't handle those to begin with so I'd prefer to handle that in another change set.
There was a problem hiding this comment.
So this line should be >= 300 || < 200, right? Because we want to consider 3xx errors too, for now.
There was a problem hiding this comment.
Yep, that's right.
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
|
@jhjaggars thanks for dealing with this issue on openshift side RedHatInsights/insights-ingress-go#133 |
|
Can we also extend the user-agent to include the cluster version in the user-agent header: we might need to make some condition on ingress to keep older clusters to work |
|
/retest |
1 similar comment
|
/retest |
If that happens, here or in another PR, can it be fed from |
Currently we put the hash of the commit in the user-agent, could we replace that with |
Signed-off-by: Jesse Jaggars <[email protected]>
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhjaggars, martinkunc 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 |
openshift-ci-robot
added
the
approved
|
/retitle Bug 1872557: handle 201 response from upload |
openshift-ci-robot
changed the title
openshift-ci-robot
added
bugzilla/severity-unspecified
|
@jhjaggars: This pull request references Bugzilla bug 1872557, which is invalid:
Comment In response to this:
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. |
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/valid-bug
|
@martinkunc: This pull request references Bugzilla bug 1872557, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
|
@jhjaggars: All pull requests linked via external trackers have merged: Bugzilla bug 1872557 has been moved to the MODIFIED state. In response to this:
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. |
|
I created separate case, CCXDEV-2918 For adding Release version to User Agent. |
|
/bugzilla refresh |
|
@martinkunc: Bugzilla bug 1872557 is in an unrecognized state (ON_QA) and will not be moved to the MODIFIED state. In response to this:
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. |
|
@jhjaggars the RELEASE_VER seems more useful to me than the commit sha: if that doesn't have any use, switching to version itself would make more sense to me, and adding the commit info as addition, if needed. |
The ingress service for cloud.redhat.com started returning 201 status codes when posting archives. This change handles that response code as a valid OK response.