Skip to content

Update data-plane-api#2430

Merged
mattklein123 merged 24 commits intoenvoyproxy:masterfrom
kyessenov:packages
Jan 26, 2018
Merged

Update data-plane-api#2430
mattklein123 merged 24 commits intoenvoyproxy:masterfrom
kyessenov:packages

Conversation

@kyessenov
Copy link
Copy Markdown
Contributor

@kyessenov kyessenov commented Jan 23, 2018

build: Apply data-plane-api protobuf refactoring to Envoy

Description:
Mechanical rewrite of bazel dependencies and imports to reflect the movement of sources.

Risk Level: Medium
Any indirect reference to protobuf type (@type:) may be affected.

Testing: No extra testing, but current tests are updated.

Docs Changes: envoyproxy/data-plane-api#421

Release Notes: Updated the protobuf references to reflect the structured packages in data-plane-api.

[Optional API Changes:] envoyproxy/data-plane-api#421

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Copy Markdown
Contributor Author

Tests are failing since for some reason ADS descriptor is not pulled into the test binary.
@htuch @rshriram any ideas why protobuf does not include the ADS service despite my repeated attempts to both adding #include "envoy/service/discovery/v2/ads.pb.h" and bazel dependency on envoy_ads.

Signed-off-by: Kuat Yessenov <kuat@google.com>
@htuch
Copy link
Copy Markdown
Member

htuch commented Jan 23, 2018

@kyessenov not sure, have you experimented with adding a message to and instantiating the messages in ads.proto explicitly in the linked target? I wonder if there is some linker badness (vague memories of this) when we have pure service defines?

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
@kyessenov
Copy link
Copy Markdown
Contributor Author

kyessenov commented Jan 23, 2018

Resolved the issue by an explicit use of a proto dummy message (AdsDummy, CdsDummy) in the failing tests.

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
name = "envoy_accesslog",
actual = "@envoy_api//envoy/service/accesslog/v2:als_cc",
)
native.bind(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to clean this up separately to avoid binding so many names...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, yes please

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would prefer not using binds (I think they might be an antipattern as they make working with multiple workspaces difficult). We should use "actual" names, since the path is more or less stable.

Copy link
Copy Markdown
Member

@htuch htuch Jan 26, 2018

Choose a reason for hiding this comment

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

So, yeah, some history here. When we first started to Bazelify Envoy, it was unclear to me how best to handle external deps from a few perspectives:

  • How would we be able to map to prebuilts at Lyft as well as use native Bazel repositories?
  • How would we redirect dependencies when we imported into Google's monorepo.

Bind made sense at that time as an indirection mechanism to provide flexibility.

The prebuilt situation has stabilized and doesn't apply here (we don't prebuild @envoy_api). The import for Google is also now moot. While it's a little neater to rebind, TBH we're sed-ing a ton during the import, so rewriting @envoy_api is trivial.

So, definitely for this dependency, which is now sprawling with binds, let's just switch to direct reference to @envoy_api everywhere.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1 if we can just do this now since this is already a giant change and mostly find/replace. I wondered why we did this in the first place, thanks for the explanation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, with a sed swoosh

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
mattklein123
mattklein123 previously approved these changes Jan 25, 2018
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 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! Will defer to @htuch and @jmillikin-stripe on Bazel. Per your comment I would love to cleanup how we do the binds, seems extremely verbose, but can do that in a follow up.

Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

epic!

@mattklein123 mattklein123 merged commit d6b630a into envoyproxy:master Jan 26, 2018
mattklein123 added a commit that referenced this pull request Jan 26, 2018
mattklein123 added a commit that referenced this pull request Jan 26, 2018
This reverts commit d6b630a.

Signed-off-by: Matt Klein <mklein@lyft.com>
mattklein123 added a commit that referenced this pull request Jan 26, 2018
This reverts commit d6b630a.

Signed-off-by: Matt Klein <mklein@lyft.com>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Description: Adds support for registering a platform KV store implementation via EngineBuilder.
Risk Level: Low
Testing: CI

Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Description: Adds support for registering a platform KV store implementation via EngineBuilder.
Risk Level: Low
Testing: CI

Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
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.

3 participants