[tools] handle commits merged without PR in deprecated script #10723

asraa · 2020-04-09T14:05:13Z

Some commits (eg f5b0294 ) were added without a PR, so the script fails. Change so that issues can still be created.

This uses the commit message and sha instead of the PR title and number.

Testing:
I successfully got:
Flag envoy.reloadable_features.http1_flood_protection added at 2020-03-02 is not ready to remove
Instead of a crash. Tweaking the script to deprecate after 1 day and printing out the issue and title give me:

envoy.reloadable_features.http1_flood_protection deprecation
commit f5b0294f0a7c2d143f6d4d94afae9a0e6f9dacf7 (http: adding response flood protection) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to alyssawilk

Signed-off-by: Asra Ali [email protected]

Signed-off-by: Asra Ali <[email protected]>

asraa · 2020-04-09T14:05:34Z

tools/deprecate_version/deprecate_version.py

+      # In this case we cannot add a user login for an assignee.
+      change_title = commit.message.split('\n')[0] # Remove sign-off mesage
+      number = ('commit %s') % commit.hexsha
+      login = None


GitPython API can only give me author name and email, so I could not assign the issue to the NamedUser login like before. GitPython experts, is there a way?

Not that I know of. Does assigning to email work? Worst case we could assign the script runner to find an owner and print that's what is going on, but it'd be nice if we could do better.

I found search_users functionality in the API -- I did guard against not finding the user, but I think (?) there's guarantee there will be a user with the email we find.

Edit: I trial ran it without creating the issue, I successfully got your login with the script now:

envoy.reloadable_features.http1_flood_protection deprecation commit f5b0294f0a7c2d143f6d4d94afae9a0e6f9dacf7 (http: adding response flood protection) introduced a runtime guarded feature. This issue tracks source code cleanup. >> Assigning to alyssawilk

Signed-off-by: Asra Ali <[email protected]>

asraa · 2020-04-13T16:45:26Z

Output when running currently:

Flag envoy.reloadable_features.http1_flood_protection added at 2020-03-02 is not ready to remove
Flag envoy.reloadable_features.strict_header_validation added at 2019-11-13 is not ready to remove
Flag envoy.reloadable_features.connection_header_sanitization added at 2019-11-18 is not ready to remove
Flag envoy.reloadable_features.strict_authority_validation added at 2019-12-10 is not ready to remove
Flag envoy.reloadable_features.reject_unsupported_transfer_encodings added at 2019-12-13 is not ready to remove
Flag envoy.reloadable_features.strict_method_validation added at 2020-01-09 is not ready to remove
Flag envoy.reloadable_features.new_http1_connection_pool_behavior added at 2020-01-29 is not ready to remove
Flag envoy.reloadable_features.new_http2_connection_pool_behavior added at 2020-01-29 is not ready to remove
Flag envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher added at 2020-02-25 is not ready to remove
Found end sentinel

No code is deprecated.

If I change the script so I deprecate after 1 Day I get:

Flag envoy.reloadable_features.http1_flood_protection added at 2020-03-02 and is safe to remove
Flag envoy.reloadable_features.strict_header_validation added at 2019-11-13 and is safe to remove
Flag envoy.reloadable_features.connection_header_sanitization added at 2019-11-18 and is safe to remove
Flag envoy.reloadable_features.strict_authority_validation added at 2019-12-10 and is safe to remove
Flag envoy.reloadable_features.reject_unsupported_transfer_encodings added at 2019-12-13 and is safe to remove
Flag envoy.reloadable_features.strict_method_validation added at 2020-01-09 and is safe to remove
Flag envoy.reloadable_features.new_http1_connection_pool_behavior added at 2020-01-29 and is safe to remove
Flag envoy.reloadable_features.new_http2_connection_pool_behavior added at 2020-01-29 and is safe to remove
Flag envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher added at 2020-02-25 and is safe to remove
Found end sentinel

envoy.reloadable_features.http1_flood_protection deprecation
commit f5b0294f0a7c2d143f6d4d94afae9a0e6f9dacf7 (http: adding response flood protection) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to [email protected]
  >> Issue already exists, not posting!
envoy.reloadable_features.strict_header_validation deprecation
#8849 (release: flipping flags) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to alyssawilk
envoy.reloadable_features.connection_header_sanitization deprecation
#8862 (Sanitize headers nominated by the Connection header) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to abaptiste
envoy.reloadable_features.strict_authority_validation deprecation
#77 (docs: small fix) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to mattklein123
envoy.reloadable_features.reject_unsupported_transfer_encodings deprecation
#9316 (http: disallow unknown transfer encodings) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to alyssawilk
envoy.reloadable_features.strict_method_validation deprecation
#9637 (http: explicitly blocking CONNECT for HTTP/1.1) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to alyssawilk
envoy.reloadable_features.new_http1_connection_pool_behavior deprecation
#9668 (Combine H1 and H2 conn_pool implementations) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to ggreenway
envoy.reloadable_features.new_http2_connection_pool_behavior deprecation
#9668 (Combine H1 and H2 conn_pool implementations) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to ggreenway
envoy.reloadable_features.ext_authz_http_service_enable_case_sensitive_string_matcher deprecation
#9715 (ext_authz: Deprecate LowerCaseStringMatcher) introduced a runtime guarded feature. This issue tracks source code cleanup.
  >> Assigning to dio

alyssawilk

Cool, thanks for the fixes - I thought we'd had setec flags before - I wonder if the way we imported them changed?

alyssawilk · 2020-04-13T16:43:28Z

tools/deprecate_version/deprecate_version.py

+      number = ('#%d') % pr
+      login = pr_info.user.login
+    else:
+      # Extract Commit message, sha, and author.


Commit - commit

alyssawilk · 2020-04-13T16:44:47Z

tools/deprecate_version/deprecate_version.py

+    else:
+      # Extract Commit message, sha, and author.
+      # In this case we cannot add a user login for an assignee.
+      change_title = commit.message.split('\n')[0]  # Remove sign-off mesage


It's not sign off but description too (if there is one) yes?

Yep -- that's right it.

alyssawilk · 2020-04-13T16:47:21Z

tools/deprecate_version/deprecate_version.py

+      # In this case we cannot add a user login for an assignee.
+      change_title = commit.message.split('\n')[0] # Remove sign-off mesage
+      number = ('commit %s') % commit.hexsha
+      login = None


Not that I know of. Does assigning to email work? Worst case we could assign the script runner to find an owner and print that's what is going on, but it'd be nice if we could do better.

Signed-off-by: Asra Ali <[email protected]>

alyssawilk

Cool, thanks for being the non-alyssar runner of our release scripts, and fixing the holes! LGTM from "this says it is doing what I think it should be doing" perspective but as far as I'm concerned I do not have python readability so mind getting a pass from someone who does? @junr03 maybe?

mattklein123 · 2020-04-18T22:06:04Z

Please merge master. Also @junr03 PTAL. Thank you!

/wait

Signed-off-by: Asra Ali <[email protected]>

junr03

Some suggestions. I think @alyssawilk is being generous re: my python readability :).

tools/deprecate_version/deprecate_version.py

junr03 · 2020-04-20T20:40:33Z

tools/deprecate_version/deprecate_version.py

+    else:
+      # Extract commit message, sha, and author.
+      # In this case we cannot add a user login for an assignee.
+      change_title = commit.message.split('\n')[0]  # Remove description.


should we constrain the length of this message a little bit beyond the newline char? I feel like people could be more verbose here than in a PR title.

Oh maybe, it looks like there's no enforced limit on the commit title. People seem to recommend 50/72 stylistically? I arbitrarily put 50.

junr03 · 2020-04-20T20:45:54Z

tools/deprecate_version/deprecate_version.py

    print(title)
    print(body)
-    print('  >> Assigning to %s' % pr_info.user.login)
+    print('  >> Assigning to %s' % (login if login else email))


I think you can used shorthand ternary here:

Suggested change

print(' >> Assigning to %s' % (login if login else email))

print(' >> Assigning to %s' % (login or email))

Not sure which one is preferred by python people these days

Signed-off-by: Asra Ali <[email protected]>

junr03

lgtm!

Signed-off-by: Spencer Lewis <[email protected]> * master: (46 commits) allow specifying the API version of bootstrap from the command line (envoyproxy#10803) config: adding connect matcher (unused) (envoyproxy#10894) Add missing dependency on `assert.h` (envoyproxy#10918) Lower heap and disk space used by kafka tests (envoyproxy#10915) [tools] handle commits merged without PR in deprecated script (envoyproxy#10723) tools: including working tree in modified_since_last_github_commit.sh diff. (envoyproxy#10911) rocketmq_proxy: implement rocketmq proxy [docs] PR template to include commit message (envoyproxy#10900) docs: breaking long word to stop content overflow. (envoyproxy#10880) Delete legacy connection pool code. (envoyproxy#10881) wasm: clarify how configuration is passed (envoyproxy#10782) issue template: clarify security/crash reporting (envoyproxy#10885) api/faq: add entry on incremental xDS. (envoyproxy#10876) router: retry overloaded requests (envoyproxy#10847) Remove inclusion of pthread.h, not needed for linux compilation (envoyproxy#10895) request_id: Add option to always set request id in response (envoyproxy#10808) xray: Use correct types for segment document output (envoyproxy#10834) router: fixing a watermark bug for streaming retries (envoyproxy#10866) http: auditing Path() calls for safety with Pathless CONNECT (envoyproxy#10851) Remove hardcoded type urls Part.2 (envoyproxy#10848) ...

fix deprecated script to handle commits that were merged without a PR

0d39849

Signed-off-by: Asra Ali <[email protected]>

asraa commented Apr 9, 2020

View reviewed changes

format py

8dbc94f

Signed-off-by: Asra Ali <[email protected]>

alyssawilk self-assigned this Apr 13, 2020

alyssawilk reviewed Apr 13, 2020

View reviewed changes

asraa added 2 commits April 13, 2020 15:15

address comments

b2323cf

Signed-off-by: Asra Ali <[email protected]>

sneaky file

21e5540

Signed-off-by: Asra Ali <[email protected]>

alyssawilk previously approved these changes Apr 13, 2020

View reviewed changes

mattklein123 assigned junr03 Apr 14, 2020

repokitteh-read-only bot added the waiting label Apr 18, 2020

Merge remote-tracking branch 'upstream/master' into fix-deprecated

2f09b72

Signed-off-by: Asra Ali <[email protected]>

asraa dismissed alyssawilk’s stale review via 2f09b72 April 20, 2020 12:45

repokitteh-read-only bot removed the waiting label Apr 20, 2020

junr03 reviewed Apr 20, 2020

View reviewed changes

address comments

efdcae3

Signed-off-by: Asra Ali <[email protected]>

junr03 approved these changes Apr 21, 2020

View reviewed changes

asraa merged commit 528287d into envoyproxy:master Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tools] handle commits merged without PR in deprecated script #10723

[tools] handle commits merged without PR in deprecated script #10723

asraa commented Apr 9, 2020 •

edited

Loading

asraa Apr 9, 2020

alyssawilk Apr 13, 2020

asraa Apr 13, 2020

asraa Apr 13, 2020

asraa commented Apr 13, 2020

alyssawilk left a comment

alyssawilk Apr 13, 2020

asraa Apr 13, 2020

alyssawilk Apr 13, 2020

asraa Apr 13, 2020

alyssawilk Apr 13, 2020

alyssawilk left a comment

mattklein123 commented Apr 18, 2020

junr03 left a comment

junr03 Apr 20, 2020

asraa Apr 21, 2020

junr03 Apr 20, 2020

junr03 left a comment

	print(' >> Assigning to %s' % (login if login else email))
	print(' >> Assigning to %s' % (login or email))

[tools] handle commits merged without PR in deprecated script #10723

[tools] handle commits merged without PR in deprecated script #10723

Conversation

asraa commented Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Apr 13, 2020

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

mattklein123 commented Apr 18, 2020

junr03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 left a comment

Choose a reason for hiding this comment

asraa commented Apr 9, 2020 •

edited

Loading