Skip to content

Add a flag to allow for continued use of the deprecated v2 api#584

Merged
mum4k merged 11 commits intoenvoyproxy:masterfrom
oschaaf:v2-compat-flag
Dec 8, 2020
Merged

Add a flag to allow for continued use of the deprecated v2 api#584
mum4k merged 11 commits intoenvoyproxy:masterfrom
oschaaf:v2-compat-flag

Conversation

@oschaaf
Copy link
Copy Markdown
Member

@oschaaf oschaaf commented Dec 2, 2020

  • Adds a new proto/cli option, which allows requesting continued use of the about to be deprecated v2 api's.
  • Adds a new fixture / integration test for the test server, as a sanity check for making sure that when we update Envoy to the
    revision that deprecated v2 things will keep working as expected, while documenting how to set up the runtime configuration.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added the waiting-for-review A PR waiting for a review. label Dec 2, 2020
@mum4k mum4k requested a review from dubious90 December 3, 2020 02:03
@mum4k
Copy link
Copy Markdown
Collaborator

mum4k commented Dec 3, 2020

@dubious90 please review and assign back to me once done.

// "emit_previous_request_delta_in_response_header" to record elapsed time between request
// arrivals.
google.protobuf.StringValue latency_response_header_name = 36;
// Set to allow usage of the v2 api (not recommended). Default is false.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can point to v2 deprecation documentation, that would help people understand why to use or not use this.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As discussed on Slack, I will follow up on this in #575

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
dubious90
dubious90 previously approved these changes Dec 3, 2020
Copy link
Copy Markdown
Contributor

@dubious90 dubious90 left a comment

Choose a reason for hiding this comment

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

LGTM, mum4k to approve

def DISABLED_test_nighthawk_client_v2_api_breaks_by_default(http_test_server_fixture):
"""Test that the v2 api breaks us when it's not explicitly requested."""
parsed_json, _ = http_test_server_fixture.runNighthawkClient([
_, _ = http_test_server_fixture.runNighthawkClient([
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit and also mildly unfamiliar language for me, but if we don't need either variable here, can we just run the function without assigning the result to anything?

@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Dec 4, 2020
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Dec 6, 2020
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Dec 7, 2020

Oh, this needs a merge / conflict resolution, let me fix that.

Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@mum4k mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Dec 7, 2020
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
@oschaaf oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Dec 7, 2020
@oschaaf
Copy link
Copy Markdown
Member Author

oschaaf commented Dec 7, 2020

@mum4k pushed 15512ef to resolve the 0 vs None issue with the bootstrap version argument.

@mum4k mum4k merged commit 2942dc9 into envoyproxy:master Dec 8, 2020
qqustc added a commit to qqustc/nighthawk that referenced this pull request Mar 17, 2021
envoyproxy#584)"

This reverts commit 2942dc9.

Signed-off-by: qqustc@gmail.com <qqin@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-for-review A PR waiting for a review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants