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.

Sign up for GitHub

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

Jump to bottom

api: remove hardcoded type urls part.1 #10479

Merged
merged 9 commits into from
Apr 3, 2020
Merged

api: remove hardcoded type urls part.1 #10479

merged 9 commits into from
Apr 3, 2020

Conversation

Shikugawa
Copy link
Member

@Shikugawa Shikugawa commented Mar 22, 2020 •
edited
Loading

Signed-off-by: shikugawa rei@tetrate.io

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

Description: #10209. There are hardcoded type URLs in resources.h. In this PR, I removed all of the dependencies of those. Now, codes that depend on that are only test code. In the future, I will remove that from test codes and completely destroy that.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

@Shikugawa
delete hardcoded type urls
7f13d33 
Signed-off-by: shikugawa <rei@tetrate.io>
@Shikugawa
cleanup
38336ca 
Signed-off-by: shikugawa <rei@tetrate.io>
htuch
htuch reviewed Mar 23, 2020
View reviewed changes
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.

In general, I like this cleanup, but would like it split into two PRs. For next steps, I would recommend against trying to do too much in terms of magic in tests. It's fine to hardcode in tests, so any template complexity etc. needs to be warranted.

source/common/upstream/cluster_manager_impl.cc Outdated Show resolved Hide resolved
source/server/lds_api.cc Outdated Show resolved Hide resolved
source/common/upstream/eds.cc
@@ -22,6 +22,8 @@ EdsClusterImpl::EdsClusterImpl(
Stats::ScopePtr&& stats_scope, bool added_via_api)
: BaseDynamicClusterImpl(cluster, runtime, factory_context, std::move(stats_scope),
added_via_api),
Envoy::Config::SubscriptionBase<envoy::config::endpoint::v3::ClusterLoadAssignment>(
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, we're now slowly drifting into the space of true multiple inheritance, which is used somewhat sparingly in Envoy. Not sure if the horse has fully bolted here, @mattklein123?

Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't really have a strong opinion on this. I guess I'm of an opinion that our default should be no, but if there is a good case for it I don't see any major issue. cc @envoyproxy/maintainers

@Shikugawa
Copy link
Member Author

@htuch That is, We should divide this PR into two, shouldn't we? First, basic cleanup of SubscriptionBase, getResourceName and so on. Second, v2 and v3 resource pause and resume with some tests. OK?

@htuch
Copy link
Member

htuch commented Mar 25, 2020

@Shikugawa yep.

@Shikugawa
only to be refactor
1983c50 
Signed-off-by: shikugawa <rei@tetrate.io>
@Shikugawa Shikugawa changed the title api: remove hardcoded type urls api: remove hardcoded type urls part.1 Mar 28, 2020
@Shikugawa
Copy link
Member Author

@htuch Destroy hardcoded type URLs on this PR. I will create Part.2 that includes hardcoded type URLs removal and some tests with v3 resource names.

@Shikugawa
Merge branch 'master' of https://github.com/envoyproxy/envoy into rem…
ff02b50 
…ove_hardcoded_type_url

Signed-off-by: shikugawa <rei@tetrate.io>
@Shikugawa
Kick CI
894ef10 
Signed-off-by: shikugawa <rei@tetrate.io>
@Shikugawa
Merge branch 'master' of https://github.com/envoyproxy/envoy into rem…
069a312 
…ove_hardcoded_type_url

Signed-off-by: shikugawa <rei@tetrate.io>
@Shikugawa
Kick CI
9cf86b3 
Signed-off-by: shikugawa <rei@tetrate.io>
@Shikugawa
fix
b1b62f2 
Signed-off-by: shikugawa <rei@tetrate.io>
htuch
htuch previously approved these changes Mar 30, 2020
View reviewed changes
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. I'd ask that we don't do this for tests, as I think it will make them harder to write/understand/debug, conciseness is not necessarily a virtue in test code.

@mattklein123
Copy link
Member

@lizan can you take a look at clang-tidy here? I think this PR should have the fix for the headers issue so I think there might be a different issue?

/wait

@Shikugawa
fi
594e32f 
Signed-off-by: shikugawa <rei@tetrate.io>
@Shikugawa
Copy link
Member Author

@mattklein123 @htuch LGTM?

htuch
htuch approved these changes Apr 3, 2020
View reviewed changes
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 1323ff2 into envoyproxy:master Apr 3, 2020
@marcomagdy
Copy link
Contributor

marcomagdy commented Apr 3, 2020 •
edited
Loading

@Shikugawa @htuch
In this PR, how does the template

template <typename Current> struct SubscriptionBase

Differ from the one in discovery_service_base.h?

Both have the same namespace and looks like they do the same thing more or less??
I'm asking because this is showing up in clang-tidy and failing CI for other PRs.

@lizan
Copy link
Member

lizan commented Apr 3, 2020

@marcomagdy This PR removees discovery_service_base.h, so if you see this failing CI for other PRs, they likely need merge master. Azure pipelines runs on merge commit so it could cause other PRs failing.

@marcomagdy
Copy link
Contributor

Interestingly, my PR failed CI after I merged from master.

This was referenced Apr 15, 2020
Closed
Merged
htuch pushed a commit that referenced this pull request Apr 22, 2020
@Shikugawa
Remove hardcoded type urls Part.2 (#10848)
e452a7b 
To resolve #10209. This is the continuation of #10479. Remove all of hard-coded type URLs, and operate subscriptions that have the same resource name.
Risk Level: Low

Signed-off-by: shikugawa <rei@tetrate.io>
penguingao pushed a commit to penguingao/envoy that referenced this pull request Apr 22, 2020
@Shikugawa @penguingao
Remove hardcoded type urls Part.2 (envoyproxy#10848)
fe915e0 
To resolve envoyproxy#10209. This is the continuation of envoyproxy#10479. Remove all of hard-coded type URLs, and operate subscriptions that have the same resource name.
Risk Level: Low

Signed-off-by: shikugawa <rei@tetrate.io>
Signed-off-by: pengg <pengg@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@htuch htuch htuch approved these changes

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Shikugawa @htuch @mattklein123 @marcomagdy @lizan