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

direct_response: use data source #10342

Merged
merged 2 commits into from
Mar 16, 2020

Conversation

kyessenov
Copy link
Contributor

Signed-off-by: Kuat Yessenov kuat@google.com

Description: switch to using a data source for a more usable API
Risk Level: low (just recently merged the API)
Testing: unit
Docs Changes: none
Release Notes:

cc @mergeconflict

Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov kyessenov requested a review from zuercher as a code owner March 11, 2020 20:02
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10342 was opened by kyessenov.

see: more, trace.

Copy link
Member

@mergeconflict mergeconflict left a comment

Choose a reason for hiding this comment

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

nice :)

@mattklein123 mattklein123 self-assigned this Mar 11, 2020
// Response data as bytes.
bytes response = 1;
// Response data as a data source.
api.v2.core.DataSource response = 1;
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking API change. You will need to add a new field for data source.

Copy link
Member

Choose a reason for hiding this comment

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

This was added in the last few days. No one is using it yet so IMO it's fine to change.

@kyessenov
Copy link
Contributor Author

Weird coverage error once again:

All tests in the same test suite must use the same test fixture
class, so mixing TEST_F and TEST in the same test suite is
illegal.  In test suite UtilityTest,
test EnsureCreateSingletonsActuallyReturnsTheSameInstances is defined using TEST_F but
test ComputeHashedVersion is defined using TEST.  You probably
want to change the TEST to TEST_F or move it to another test
case.

@mattklein123
Copy link
Member

Please merge master and it will fix that problem

@kyessenov
Copy link
Contributor Author

Merged, still failing :( This time it looks legitimate, but I don't think I added any extra code paths.

@mattklein123
Copy link
Member

@kyessenov hmm, dunno. I think the coverage build is pretty messed up right now and I wonder if the retries is causing strange results. Maybe we should remove that. cc @lizan @htuch

/wait

@kyessenov
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #10342 (comment) was created by @kyessenov.

see: more, trace.

@kyessenov
Copy link
Contributor Author

PTAL. Coverage test de-flaked.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit abb575a into envoyproxy:master Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants