runtime/client: add util func to check conn options compatibility with env #594
+36
−4
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
aryan9600
requested review from
darkowlzz,
hiddeco and
stefanprodan
as code owners
hiddeco
reviewed
Jun 26, 2023
darkowlzz
reviewed
Jun 26, 2023
| // CheckEnvironmentCompatibility checks if the enviornment is compatible with | ||
| // the configured connection options. | ||
| func (o *ConnectionOptions) CheckEnvironmentCompatibility() error { | ||
| if !o.AllowHTTP { |
There was a problem hiding this comment.
Maybe inverse this condition to reduce the levels of nested conditions in the function?
if o.AllowHTTP {
return nil
}
...There was a problem hiding this comment.
i prefer to keep it the current way since it keeps all the logic related to the o.AllowHTTP field in a single block making it relatively easy for this function to be expanded for when we add more options.
darkowlzz
reviewed
Jun 26, 2023
|
|
||
| // CheckEnvironmentCompatibility checks if the enviornment is compatible with | ||
| // the configured connection options. | ||
| func (o *ConnectionOptions) CheckEnvironmentCompatibility() error { |
There was a problem hiding this comment.
Could this function be called Validate() ?
There was a problem hiding this comment.
i prefer the current name over Validate() as imo, the latter means that its checking the configured options are valid which is a bit misleading.
aryan9600
force-pushed
the
http-env-var
branch
3 times, most recently
from
eca38b5 to
6ea71a1
Compare
makkes
reviewed
Jun 27, 2023
aryan9600
force-pushed
the
http-env-var
branch
4 times, most recently
from
609fbf9 to
71dd9a0
Compare
stefanprodan
added
the
area/runtime
hiddeco
reviewed
Jun 29, 2023
…h env Add `CheckEnvironmentCompatibility()` to check whether the configured connection options are compatible with the environment, by checking env vars like `HTTP_PROXY` and `HTTPS_PROXY`. Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
hiddeco
approved these changes
Jun 29, 2023
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.
Add
CheckEnvironmentCompatibility()to check whether the configured connection options are compatible with the environment, by checking env vars likeHTTP_PROXYandHTTPS_PROXY.