dep: Remove dependency - six by kfaseela · Pull Request #19085 · envoyproxy/envoy

kfaseela · 2021-11-23T15:19:01Z

Signed-off-by: Faseela K faseela.k@est.tech

Commit Message: Remove dependency - six
Additional Description: An internal 3pp scan reports that six version is older and a newer version is available. However as six is no longer used, attempting to remove the same
Risk Level: low
Testing: local build and pre checks done

repokitteh-read-only · 2021-11-23T15:19:06Z

Hi @kfaseela, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19085 was opened by kfaseela.

see: more, trace.

repokitteh-read-only · 2021-11-23T15:19:10Z

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @htuch

🐱

Caused by: #19085 was opened by kfaseela.

see: more, trace.

phlax

i wonder if this dep is required any longer, but otherwise lgtm

moderation · 2021-11-23T17:22:27Z

I created protocolbuffers/protobuf#5531 but it looks like six has now been removed from protocolbuffers - protocolbuffers/protobuf#9096

We don't support Python 2 either so removal would be best. Taking a look now

kfaseela · 2021-11-23T18:21:53Z

I created protocolbuffers/protobuf#5531 but it looks like six has now been removed from protocolbuffers - protocolbuffers/protobuf#9096

We don't support Python 2 either so removal would be best. Taking a look now

I can update the PR by removing the dependency if it is okay for everyone.

moderation · 2021-11-23T18:26:57Z

I did a quick non-test build with six removed and it worked. I suspect the problem will be with the Thrift filter tests due to archaic Python code

test/extensions/filters/network/thrift_proxy/thrift_object_impl_test.cc
70:            value = "six";                                          
125:      EXPECT_EQ("six", value.getValueTyped<std::string>());        
                                                                       
test/extensions/filters/network/thrift_proxy/requirements.in           
1:six                                                                  
                                                                       
test/extensions/filters/network/thrift_proxy/driver/BUILD              
27:        requirement("six"),                                         
40:        requirement("six"),

phlax · 2021-11-23T18:32:57Z

thrift has its own requirements with six listed so i think that wont be an issue

@kfaseela i would say go ahead and remove it

phlax · 2021-11-23T18:34:44Z

test/extensions/filters/network/thrift_proxy/thrift_object_impl_test.cc

hmm, i see, its using it outside of the py rules

Signed-off-by: Faseela K <faseela.k@est.tech>

moderation · 2021-11-23T19:04:10Z

Yes, I think this C++ test code is referencing the six dependency in bazel/repository_locations,bzl and bazel/repositories,bzl. Related to this is the Thrift tech debt issue - #10701

moderation · 2021-11-23T19:25:30Z

It looks like #10702 was closed before merging. References to PY2 are still present

test/extensions/filters/network/thrift_proxy/driver/BUILD
21:    python_version = "PY2",                           
34:    python_version = "PY2",

phlax · 2021-11-23T19:44:39Z

It looks like #10702 was closed before merging

following up on that pr it seems like the things that it patched still would require patching

@moderation what is your thought on the best way forward update the dep or drop it ?

moderation · 2021-11-23T21:43:08Z

It would be preferable to drop it. ...Don't have to maintain a dependency that isn't there 😀

Even though we've made real progress on dep currency over the last couple of months, @kfaseela is going to find quite a few more out of date deps that are harder to bump and require deeper code change. fmt and spdlog are in this category

I'd like to try some local tests to see if I can remove six

kfaseela · 2021-11-24T04:56:33Z

It would be preferable to drop it. ...Don't have to maintain a dependency that isn't there 😀

Even though we've made real progress on dep currency over the last couple of months, @kfaseela is going to find quite a few more out of date deps that are harder to bump and require deeper code change. fmt and spdlog are in this category

I'd like to try some local tests to see if I can remove six

First time contributor, hence a basic doubt :) why did all the pre checks pass? Or we are yet to run the tests on the PR?

phlax · 2021-11-24T08:08:14Z

why did all the pre checks pass? Or we are yet to run the tests on the PR?

not sure, but it seems like the bit of code that uses six is not being hit in the CI tests

the "presubmit" tests are the PR tests

kfaseela · 2021-11-24T11:22:11Z

why did all the pre checks pass? Or we are yet to run the tests on the PR?

not sure, but it seems like the bit of code that uses six is not being hit in the CI tests

the "presubmit" tests are the PR tests

Ok. somehow, even running the thrift test locally passed for me. I did a Bazel clean build of envoy on my MacBook with six dependency removed, and then ran "bazel test //test/extensions/filters/network/thrift_proxy:thrift_object_impl_test" and it PASSED. May be I missed something!

moderation · 2021-11-24T21:55:41Z

I got the same results @kfaseela !! All tests pass. Looking closer at the code I don't believe the Thrift tests leverage the C++ package six. So I think this is good to go and great to remove unnecessary deps. I'm guessing that as protobuf dropped support we no longer need to pull in.

/lgtm deps

kfaseela · 2021-11-25T05:56:29Z

I got the same results @kfaseela !! All tests pass. Looking closer at the code I don't believe the Thrift tests leverage the C++ package six. So I think this is good to go and great to remove unnecessary deps. I'm guessing that as protobuf dropped support we no longer need to pull in.

/lgtm deps

Thank you! should I push a docs PR to remove six from here? https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/security/external_deps

phlax · 2021-11-25T07:53:11Z

Thank you! should I push a docs PR to remove six from here?

no, that should be automatically generated

phlax

lgtm, thanks @kfaseela

rojkov

Thank you @kfaseela!

kfaseela · 2021-11-26T10:09:28Z

@rojkov I see different roles labelled, for people who are submitting PRs. What is the eligibility criteria for becoming a "Member" from a "Contributor" ?

rojkov · 2021-11-26T11:21:06Z

@rojkov I see different roles labelled, for people who are submitting PRs. What is the eligibility criteria for becoming a "Member" from a "Contributor" ?

You can check https://github.com/envoyproxy/envoy/blob/main/GOVERNANCE.md

* main: (77 commits) Fix verify_and_print_latest_release logic (envoyproxy#19111) http2: drain only once when reached max_requests_per_connection (envoyproxy#19078) Overload: Reset H2 server stream only use codec level reset mechanism (envoyproxy#18895) Update QUICHE from c2ddf95dc to 7f2d442e3 (envoyproxy#19095) tools: Fix dependency checker release dates bug (envoyproxy#19109) cve_scan: Use `envoy.dependency.cve_scan` (envoyproxy#19047) tcp: fix overenthusiastic bounds on the new pool (envoyproxy#19036) dep: update Proxy-Wasm C++ host (2021-11-18). (envoyproxy#19074) build(deps): bump frozendict from 2.0.7 to 2.1.0 in /tools/base (envoyproxy#19080) kafka: dependency upgrades (envoyproxy#18995) build(deps): bump charset-normalizer in /tools/dependency (envoyproxy#19105) build(deps): bump slack-sdk in /.github/actions/pr_notifier (envoyproxy#19093) dep: Remove dependency - six (envoyproxy#19085) Remove requested_server_name_ field from StreamInfo (envoyproxy#19102) broken link path fix for items http_filters/grpc_json_transcoder_filter (envoyproxy#19101) quic: turn off GRO (envoyproxy#19088) Listener: Add global conn limit opt out. (envoyproxy#18876) Specify type for matching Subject Alternative Name. (envoyproxy#18628) Fix a broken example in Lua filter docs (envoyproxy#19086) Fix a small typo (envoyproxy#19058) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Nov 23, 2021

repokitteh-read-only bot assigned htuch Nov 23, 2021

phlax previously approved these changes Nov 23, 2021

View reviewed changes

dep: Remove dependency - six

0896e36

Signed-off-by: Faseela K <faseela.k@est.tech>

kfaseela dismissed phlax’s stale review via 0896e36 November 23, 2021 19:05

kfaseela force-pushed the upgrade-six branch from 1cede2f to 0896e36 Compare November 23, 2021 19:05

kfaseela changed the title ~~dep: Upgrade dependency - six~~ dep: Remove dependency - six Nov 23, 2021

repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Nov 24, 2021

phlax approved these changes Nov 25, 2021

View reviewed changes

rojkov approved these changes Nov 26, 2021

View reviewed changes

rojkov merged commit bfde85e into envoyproxy:main Nov 26, 2021

kfaseela deleted the upgrade-six branch January 13, 2022 16:24

Conversation

kfaseela commented Nov 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 23, 2021

Uh oh!

repokitteh-read-only bot commented Nov 23, 2021

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

moderation commented Nov 23, 2021

Uh oh!

kfaseela commented Nov 23, 2021

Uh oh!

moderation commented Nov 23, 2021

Uh oh!

phlax commented Nov 23, 2021

Uh oh!

phlax commented Nov 23, 2021

Uh oh!

moderation commented Nov 23, 2021

Uh oh!

moderation commented Nov 23, 2021

Uh oh!

phlax commented Nov 23, 2021

Uh oh!

moderation commented Nov 23, 2021

Uh oh!

kfaseela commented Nov 24, 2021

Uh oh!

phlax commented Nov 24, 2021

Uh oh!

kfaseela commented Nov 24, 2021

Uh oh!

moderation commented Nov 24, 2021

Uh oh!

kfaseela commented Nov 25, 2021

Uh oh!

phlax commented Nov 25, 2021

Uh oh!

phlax left a comment

Choose a reason for hiding this comment

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

kfaseela commented Nov 26, 2021

Uh oh!

rojkov commented Nov 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kfaseela commented Nov 23, 2021 •

edited

Loading