-
Notifications
You must be signed in to change notification settings - Fork 210
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
Add scheme option to bind to both HTTP and HTTPS #1215
Merged
Merged
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
0e467a4
Add scheme option to bind to both HTTP and HTTPS
raz-amir ea0ba66
changes
raz-amir b053e10
Change to 2 listeners and a single server
raz-amir d708fee
fix
raz-amir 46a6906
Apply suggestions from code review
raz-amir 9cffba9
fix for default ports handling
raz-amir 8614c04
check if flags default value using visit
raz-amir da920f7
add example test runs and unit tests
raz-amir File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this be a breaking change? Like, if I'm currently using fake-gcs-server like this:
The server will now start on port
8000
instead of8080
.I know this is fairly complicated, but could we make it so:
port
andport-http
no longer have a default at the flag definition. We'll handle it ourselves-scheme http
and-port
, we use that port (keep current behavior)-scheme http
, we use the default-port
(keep current behavior)-scheme both
, we'll use the default-port
and-port-http
-port
and-scheme https
Perhaps we can turn each of the 5 examples in test cases for the function/core logic that handles this.
We may be able to implement this custom logic with a custom implementation of flag.Value, but it sounds tricky as it'd need to be aware of the value of another flag. The biggest advantage of using a custom flag.Value is differentiating between the user not providing the flag and setting it to
0
(I'm not super concerned about that though - right now if the user sets it to0
, we start the server in a random port, but never print that port to the user, so who cares? heh)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct. I overlooked the http port in my last fix and broke the behavior, that I previously maintained. Thanks for catching that!
I implemented the
port
andport-http
without defaults in theflag
API but set it myself in the code if their value is 0.As you suggest, I also think that no one uses it today with port 0, so I don't think it is a breaking change, so I didn't implement the customflag.Value
.I have an idea of how to implement it, maybe using
flag.Visit
, and I will do it if you think it is needed.Please let me know.
Implemented after all using
flag.Visit
- so the port 0 can be set by a user as before.Will work on adding the test cases soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fsouza
Added unit tests to cover the examples you listed and few more to be on the safe side. The tests found an issue with
both
scheme in thecfg.externalURL
config - fixed.Added 2 more runs to the github action to cover
both
scheme with default and non-default ports.Let me know what you think. Thanks