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

Convert cluster-related v2 API test fragments to v3 #11031

Merged
merged 5 commits into from
Jun 26, 2020

Conversation

ankatare
Copy link
Contributor

@ankatare ankatare commented May 1, 2020

Signed-off-by: Abhay Narayan Katare [email protected]

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: changes in test files for "Convert v2 API test fragments to v3"
Additional Description: file changes in test/common/upstream/
Risk Level:
Testing: format test and integration test
Docs Changes:
Release Notes:
[Optional Runtime guard:]
Progress toward #10843
[Optional Deprecated:]
Note : I will push remaining files changes in next PR

@zuercher
Copy link
Member

zuercher commented May 1, 2020

@junr03 can you have a look?

Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

Hey @ankatare thanks for contributing! A comment about your CI failure.

Also do you mind updating the PR description per the docs here?

test/common/upstream/cluster_manager_impl_test.cc Outdated Show resolved Hide resolved
@junr03
Copy link
Member

junr03 commented May 11, 2020

@ankatare do you need guidance on how to proceed here? It looks like you need to fix the field differences between the v2 and the v3 protos. Looking at the errors in the build should guide you there. Let me know if you need anything!

@ankatare
Copy link
Contributor Author

@junr03 Hi Thanks for your guidance. I fixed most of the build issues, currently i am working on some issues in my local env.
will get back to you soon for anything.

@ankatare
Copy link
Contributor Author

@junr03 seems some formatting issue here. need your help to understand it and how to follow guidelines.

@ankatare
Copy link
Contributor Author

@junr03 i am talking about current build ( ongoing ).

@dio
Copy link
Member

dio commented May 12, 2020

@ankatare I think you can try to run:

./tools/code_format/check_format.py fix test/common/upstream/cluster_manager_impl_test.cc

Or (to fix all possible files),

./tools/code_format/check_format.py fix

And here is the diff, after running the above command:

diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc
index 2b922ec09..cf718a40f 100644
--- a/test/common/upstream/cluster_manager_impl_test.cc
+++ b/test/common/upstream/cluster_manager_impl_test.cc
@@ -104,7 +104,7 @@ public:
     auto message_ptr = admin_.config_tracker_.config_tracker_callbacks_["clusters"]();
     const auto& clusters_config_dump =
         dynamic_cast<const envoy::admin::v3::ClustersConfigDump&>(*message_ptr);
-
+
     envoy::admin::v3::ClustersConfigDump expected_clusters_config_dump;
     TestUtility::loadFromYaml(expected_dump_yaml, expected_clusters_config_dump);
     EXPECT_EQ(expected_clusters_config_dump.DebugString(), clusters_config_dump.DebugString());
diff --git a/test/common/upstream/utility.h b/test/common/upstream/utility.h
index d2401caeb..f66353195 100644
--- a/test/common/upstream/utility.h
+++ b/test/common/upstream/utility.h
@@ -48,7 +48,6 @@ constexpr static const char* kDefaultStaticClusterTmpl = R"EOF(
   }
   )EOF";

-
 inline std::string defaultStaticClusterJson(const std::string& name) {
   return fmt::sprintf(kDefaultStaticClusterTmpl, name, R"EOF(
 "socket_address": {

For completeness, this is the log output from CI: https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/39431/logs/7

@ankatare
Copy link
Contributor Author

@dio Thanks for help. let me run script locally

@ankatare
Copy link
Contributor Author

@dio though build is still running, let's wait for final results.

@ankatare ankatare requested a review from junr03 May 13, 2020 11:51
@ankatare
Copy link
Contributor Author

ankatare commented May 14, 2020

@dio @junr03 need your help to understand the build issue ..

"Caused by: com.google.devtools.build.lib.remote.ExecutionStatusException: FAILED_PRECONDITION: no matching bots available"

Another Message: mesg: ttyname failed: Inappropriate ioctl for device

@ankatare
Copy link
Contributor Author

@zuercher Hi, Thanks for help. finally it merged :)

@stale
Copy link

stale bot commented May 25, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 25, 2020
Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

@ankatare now that what you have is green I was wondering if you were going to tackle the rest of the fragments in the test/ tree. Otherwise, we can merge this, but not call #10843 fixed; as this PR does not quite fix all of it.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 28, 2020
@ankatare
Copy link
Contributor Author

@junr03 thanks for your comments. I am currently working on review comments on #11213
Moreover #11213 also contains file changes wrt #11031
I will continue pushing PRs in order to fix #10843.

junr03
junr03 previously approved these changes Jun 4, 2020
Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

I updated the PR description. Thanks for working on this!

@junr03
Copy link
Member

junr03 commented Jun 4, 2020

@envoyproxy/senior-maintainers I think this PR is jammed because it predates the CI changes for coverage so the status is not being reported. Can one of you with admin merge this?

@mattklein123
Copy link
Member

Please merge master to make sure we don't have other issue, thanks!

/wait

@stale
Copy link

stale bot commented Jun 12, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 12, 2020
@junr03
Copy link
Member

junr03 commented Jun 12, 2020

@ankatare do you mind merging master here?

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 12, 2020
@ankatare
Copy link
Contributor Author

@junr03 sorry , I didn't get. What is ming merging ?

@junr03
Copy link
Member

junr03 commented Jun 12, 2020

@junr03 sorry , I didn't get. What is ming merging ?

typo s/ming/mind

@junr03
Copy link
Member

junr03 commented Jun 12, 2020

I'd like a master merge on this branch to make sure all the new CI flows run correctly.

@ankatare
Copy link
Contributor Author

@junr03 ok , sure. Thanks for inputs. I will merge it ASAP.

@ankatare
Copy link
Contributor Author

@junr03 done as per inputs. Thanks

@junr03
Copy link
Member

junr03 commented Jun 17, 2020

@ankatare did you push the merge? I don't see it here

@ankatare
Copy link
Contributor Author

@junr03 ohh, really sorry. will do it ASAP today

ankatare added 4 commits June 18, 2020 13:15
Signed-off-by: Abhay Narayan Katare <[email protected]>
Signed-off-by: Abhay Narayan Katare <[email protected]>
Signed-off-by: Abhay Narayan Katare <[email protected]>
Signed-off-by: Abhay Narayan Katare <[email protected]>
Signed-off-by: Abhay Narayan Katare <[email protected]>
@ankatare
Copy link
Contributor Author

@junr03 Done now.

@stale
Copy link

stale bot commented Jun 26, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 26, 2020
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 26, 2020
@junr03 junr03 changed the title Convert v2 API test fragments to v3 Convert cluster-related v2 API test fragments to v3 Jun 26, 2020
@junr03 junr03 merged commit 19f5803 into envoyproxy:master Jun 26, 2020
@ankatare ankatare deleted the v2_to_v3_gragment_change branch July 14, 2020 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants