🩹 pd: fix auto-https default grpc address #3650
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
this conditional hinges on the wrong option.
currently, we have this table of outcomes:
```
grpc_bind|Some(grpc) |None |
auto-https |-----------|------------|
on |grpc |0.0.0.0:8080|
off |grpc |0.0.0.0:8080|
```
change this to follow what the comments and documentation state as the
intent of this code: change the default grpc address based on whether we
have an auto-managed https domain.
the `parse` calls below only refer to hardcoded values defined in the source. let's define our defaults as constants, so that we do not have error-handling logic that only refers to what ought to be known-good addresses.
cratelyn
commented
Jan 23, 2024
crates/bin/pd/src/main.rs
Outdated
| let grpc_bind = if grpc_bind.is_some() { | ||
| let grpc_bind = if grpc_auto_https.is_some() { |
There was a problem hiding this comment.
refactors are done in a separate commit to highlight ☝️ this is as the core mechanical issue at hand.
|
Nice, thanks for fixing! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
while working on #3627, i found that a conditional hinges on the wrong option.
this changes pd to follow what the comments and documentation state as the
intent of this code: change the default grpc address based on whether we
have an auto-managed https domain.
a second commit refactors the control flow for the default address. the
parsecalls only refer to hardcoded values defined in the source. let's define our
defaults as constants, so that we do not have error-handling logic that only
refers to what ought to be known-good addresses.