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

Dependencies: refactor API dependencies, fix proto JSON block, finish pip install hashes #13343

Merged
merged 14 commits into from
Oct 2, 2020

Conversation

moderation
Copy link
Contributor

@moderation moderation commented Oct 1, 2020

Signed-off-by: moderation michael@sooper.org

Risk Level: Low
Testing:

  • bazel --nohome_rc build --jobs 12 --ui_actions_shown 12 @envoy_api_canonical//envoy/...
  • docs/build.sh

Docs Changes: none required
Release Notes: none required

Comment: Will enable addition of API dependencies in follow up to #13340 /cc @htuch

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-watchers: FYI only for changes made to (bazel/repository_locations\.bzl)|(api/bazel/repository_locations\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #13343 was opened by moderation.

see: more, trace.

Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
…xtra.bzl

Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
Signed-off-by: Michael Payne <michael@sooper.org>
htuch
htuch previously approved these changes Oct 1, 2020
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.

Really nice, excited to have all proxy binary deps now with metadata.

@moderation
Copy link
Contributor Author

@htuch I'm a little stuck as bumping the Pygments (2.6.0 to 2.7.1) Python dependency introduces more explicit JSON escaping. So docs fail on the following JSON snippets in stats.proto

    // .. code-block:: json
    //   {
    //     "tag_name": "envoy.cluster_name",
    //     "regex": "^cluster\.((.+?)\.)"
    //   }
    // .. code-block:: json
    //   [
    //     {
    //       "tag_name": "envoy.http_user_agent",
    //       "regex": "^http(?=\.).*?\.user_agent\.((.+?)\.)\w+?$"
    //     },
    //     {
    //       "tag_name": "envoy.http_conn_manager_prefix",
    //       "regex": "^http\.((.*?)\.)"
    //     }
    //   ]

But when fixing the snippets with \\ vs \ Run check format scripts breaks - https://dev.azure.com/cncf/envoy/_build/results?buildId=52723&view=logs&j=9fd74226-df5b-5f29-39da-76fc341cc763&t=068b98b1-acf0-52d0-fc6e-f6d1a5b1d5ef. I'm not able to repro locally with env CLANG_FORMAT=/usr/local/bin/clang-format tools/code_format/check_format.py check so still digging

@htuch
Copy link
Member

htuch commented Oct 1, 2020

@moderation is this just for Sphinx generation? In any case, I recommend splitting this Pygments upgrade into a separate PR if it's going to involve a lot of reasoning.

… bump and sync to generated protos this time

Signed-off-by: Michael Payne <michael@sooper.org>
@moderation
Copy link
Contributor Author

I think I've worked this out and could produce locally. Trying one me time

@moderation
Copy link
Contributor Author

This is ready for final review @htuch + @envoyproxy/api-shepherds


# Interpolate {version} in the above dependency specs. This code should be capable of running in both Python
# and Starlark.
def _dependency_repositories():
Copy link
Member

Choose a reason for hiding this comment

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

FYI I will refactor this with bazel/repository_locations.bzl in a followup where APIs are added to the dashboard.

@@ -201,7 +201,7 @@ message TagSpecifier {
//
// {
// "tag_name": "envoy.cluster_name",
// "regex": "^cluster\.((.+?)\.)"
// "regex": "^cluster\\.((.+?)\\.)"
Copy link
Member

Choose a reason for hiding this comment

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

Is this a docs only change? Will the docs be misleading vs. config users need to plugin?

Copy link
Contributor Author

@moderation moderation Oct 2, 2020

Choose a reason for hiding this comment

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

This should be a doc only change. The increased escaping enforcement in Pygment caught the issue with the existing code. What is there is not valid JSON. This is the only error produced in a full docs run with the new Python deps
image
image

The current invalid JSON in the docs is rendered today at https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/metrics/v3/stats.proto#envoy-v3-api-field-config-metrics-v3-tagspecifier-regex

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 but a question on the escaping 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!

@repokitteh-read-only repokitteh-read-only bot removed the api label Oct 2, 2020
@htuch
Copy link
Member

htuch commented Oct 2, 2020

/lgtm v2-freeze

@htuch htuch merged commit 4b4dc5c into envoyproxy:master Oct 2, 2020
@alyssawilk
Copy link
Contributor

Hey @moderation thanks for making deps more clear!
I think this may have broken the deprecation versions script though - sh tools/deprecate_version/deprecate_version.sh now complains about having non-pinned deps. Mind taking a look?

@moderation
Copy link
Contributor Author

moderation commented Oct 7, 2020

Sorry for this. Somehow this script isn't triggered in CI. I'll raise a PR ASAP to fix

Deprecated==1.2.10 \
    --hash=sha256:525ba66fb5f90b07169fdd48b6373c18f1ee12728ca277ca44567a367d9d7f74 \
    --hash=sha256:a766c1dccb30c5f6eb2b203f87edd1d8588847709c78589e1521d769addc8218
gitdb==4.0.5 \
    --hash=sha256:91f36bfb1ab7949b3b40e23736db18231bf7593edada2ba5c3a174a7b23657ac \
    --hash=sha256:c9e1f2d0db7ddb9a704c2a0217be31214e91a4fe1dea1efad19ae42ba0c285c9
GitPython==3.1.8 \
    --hash=sha256:080bf8e2cf1a2b907634761c2eaefbe83b69930c94c66ad11b65a8252959f912 \
    --hash=sha256:1858f4fd089abe92ae465f01d5aaaf55e937eca565fb2c1fce35a51b5f85c910
PyGithub==1.53 \
    --hash=sha256:776befaddab9d8fddd525d52a6ca1ac228cf62b5b1e271836d766f4925e1452e \
    --hash=sha256:8ad656bf79958e775ec59f7f5a3dbcbadac12147ae3dc42708b951064096af15
smmap==3.0.4 \
    --hash=sha256:54c44c197c819d5ef1991799a7e30b662d1e520f2ac75c9efbeb54a742214cf4 \
    --hash=sha256:9c98bbd1f9786d22f14b3d4126894d56befb835ec90cef151af566c7e19b5d24
wrapt==1.12.1 \
    --hash=sha256:b62ffa81fb85f4332a4f609cab4ac40709470da05643a082ec1eb88e6d9b97d7

PR: #13433

@alyssawilk
Copy link
Contributor

No worries, as you say it doesn't run in CI because of the credentials issue (we run manually when we cut the release). I suppose we could have an early exit version which didn't require the credentials just to catch things like this but it's generally not a biggie to just fix.

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.

None yet

3 participants