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

#5888 cleanup old env vars #7911

Closed

Conversation

pnikonowicz
Copy link

cleans up the ALPN enabled env var via 2 commits:

  1. the actual change
  2. a refactoring to support the change. this has many white space changes and would be easiest to review via ignoring white space

this does not address all of #5888 and therefore should remain open

part of issue grpc#5888

removes the ability to toggle ALPN
enforcment as the default is true
and there is a development policy
in place that when env values
become true by default, the toggle
behavior should be removed
now that ALPN is always enforced,
the table driven tests that support
this can be removed.

most of the changes are whitespace,
however an assertion has been modified
so that the tests will fail if an expected
error is not returned
Copy link

linux-foundation-easycla bot commented Dec 10, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.07%. Comparing base (66ba4b2) to head (19bbbc1).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7911      +/-   ##
==========================================
+ Coverage   82.04%   82.07%   +0.03%     
==========================================
  Files         377      377              
  Lines       38180    38175       -5     
==========================================
+ Hits        31326    31334       +8     
+ Misses       5551     5542       -9     
+ Partials     1303     1299       -4     
Files with missing lines Coverage Δ
credentials/tls.go 88.19% <100.00%> (+1.61%) ⬆️
internal/envconfig/envconfig.go 100.00% <ø> (ø)

... and 16 files with indirect coverage changes

@arjan-bal arjan-bal self-requested a review December 10, 2024 09:59
@arjan-bal arjan-bal self-assigned this Dec 10, 2024
@arjan-bal
Copy link
Contributor

Hi @pnikonowicz, thanks for the PR. We can't remove the environment variable for ALPN enforcement as some users need a way to disable this for backward compatibility, see #7769

I've mentioned the same in #5888. Closing this PR for now.

@arjan-bal arjan-bal closed this Dec 10, 2024
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.

2 participants