Bug 1866743: api: Detect spec version to serve from Accept header #1960
Conversation
LorbusChris
force-pushed
the
api-use-accept-header
branch
from
2888902 to
33414c4
Compare
openshift-ci-robot
added
the
approved
LorbusChris
force-pushed
the
api-use-accept-header
branch
from
33414c4 to
c2412bc
Compare
|
/lgtm |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
LorbusChris
force-pushed
the
api-use-accept-header
branch
from
c2412bc to
6ce51c0
Compare
|
fixed the comment, needs another lgtm |
openshift-ci-robot
removed
the
do-not-merge/hold
|
This will work for the Accept header currently generated by Ignition, but isn't really a valid Accept parser. I don't know how much that matters to us. If third-party clients (curl scripts, etc.) will also be generating headers, they may find this parser brittle. Proposal:
Steps 4 and 5 violate the stated intent of the header (there's a wildcard match at the end, implying that Ignition wants to receive whatever it can get, and takes responsibility for failing if it doesn't like it), but avoid having to parse wildcards. |
LorbusChris
force-pushed
the
api-use-accept-header
branch
from
6ce51c0 to
3ab0c77
Compare
|
@bgilbert I've changed this according to your proposal. PTAL |
LorbusChris
changed the title
|
just adding hold as we are post FF /hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
It was approved before FF, does that mean it's ok to go in? Or would it have needed the lgtm as well before? |
|
/retest |
A non-BZ PR needs to be final and fully approved (approved and lgtm) by the FF deadline. This only had an approved tag and also was updated/pushed to today (the 3rd day after the deadline) and requires a review. |
|
This fixes wrong but working behavior. I'll create a BZ. |
looks good from my perspective @LorbusChris i'll let the others verify just please add the BZ if you'd like the hold on this removed to merge. 😸 |
|
I don't see Prow enforcing the BZs right now, so IMO let's not go through the overhead of filing a new bug - this is just a bugfix to Ignition spec 3 migration right? |
This is not about prow, Christian already said he'd be attaching a BZ. We are asking everyone to follow the rules on this to ensure we have no problems as we have been doing every release cycle. |
LorbusChris
changed the title
openshift-ci-robot
added
the
bugzilla/severity-unspecified
|
@LorbusChris: This pull request references Bugzilla bug 1866743, 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. |
openshift-ci-robot
added
the
bugzilla/invalid-bug
|
/bugzilla refresh |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@LorbusChris: This pull request references Bugzilla bug 1866743, which is valid. The bug has been moved to the POST state. 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. |
openshift-ci-robot
removed
the
bugzilla/invalid-bug
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/severity-high
|
@ashcrow: This pull request references Bugzilla bug 1866743, which is valid. 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. |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bgilbert, cgwalters, LorbusChris 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
LGTM as well! As a follow up PR maybe it'd be nice to have a snippet in the docs to show what an example request would look like if you were to use curl yourself. The minimum accepted header seems to be |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@LorbusChris: The following tests failed, say
Full PR test history. Your PR dashboard. 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 understand the commands that are listed here. |
|
@LorbusChris: All pull requests linked via external trackers have merged: openshift/machine-config-operator#1960. Bugzilla bug 1866743 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 ran into this question while verifying openshift#1960, so let's document it
- What I did
Makes the API inspect the Accept header instead of the User-Agent to
derive the Ignition config spec version it has to serve.
- How to verify it
e2e CI
- Description for the changelog
api: Derive spec version to serve from Accept header instead of User-Agent