Skip to content

fix: error if protocol isn't specified in --access-url #4835

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Nov 1, 2022
Merged

fix: error if protocol isn't specified in --access-url #4835

merged 2 commits into from
Nov 1, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented Nov 1, 2022

Closes #4783

kylecarbs
kylecarbs reviewed Nov 1, 2022
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change that although is easy to fix, we might wanna make sure our docs don't have any usage without a protocol.

The primary case I've seen is --access-url localhost:8080 or something.

I think it's maybe better if we default to HTTP instead.

@f0ssel f0ssel requested a review from a team as a code owner November 1, 2022 15:21
@f0ssel f0ssel requested review from jsjoeio and removed request for a team November 1, 2022 15:21
@f0ssel
Copy link
Contributor Author

f0ssel commented Nov 1, 2022

Yeah I personally like the breaking change and requiring it just because it's no extra work to provide that information.

@f0ssel
Copy link
Contributor Author

f0ssel commented Nov 1, 2022

Specifically, I would rather hard error because any sort of inference we make can lead to situations like this - #2874 (comment)

@kylecarbs
Copy link
Member

Fair enough

@f0ssel
Copy link
Contributor Author

f0ssel commented Nov 1, 2022

I'll talk with ben and see if we can make this breaking change loud in the changelog

jsjoeio
jsjoeio approved these changes Nov 1, 2022
View reviewed changes
Copy link
Contributor

@jsjoeio jsjoeio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the FE ✅

@bpmct
Copy link
Member

bpmct commented Nov 1, 2022

Garrett and I synced. We'll include these breaking changes at the top of the changelog

@f0ssel f0ssel merged commit ddbae4d into main Nov 1, 2022
@f0ssel f0ssel deleted the f0ssel/error-missing-scheme branch November 1, 2022 16:59
@github-actions github-actions bot locked and limited conversation to collaborators Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsjoeio jsjoeio jsjoeio approved these changes

@kylecarbs kylecarbs kylecarbs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

access URL defaults to HTTPS when defined without a protocol
4 participants
@f0ssel @kylecarbs @bpmct @jsjoeio