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

feat(v2): add support for key,cert in https #3594

Merged
merged 6 commits into from
Oct 26, 2020

Conversation

ThakurKarthik
Copy link
Contributor

Motivation

Adding support for https with key and cert files in docusaurus

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

1.Install mkcert anywhere to create certificates for local testing
2.In the root folder of docusaurus run mkcert localhost 127.0.0.1 ::1. mkcert will generate two files, one is key file and other cert file for testing.Move these two files to website folder.
3.Run HTTPS=true SSL_CRT_FILE=localhost+2.pem SSL_KEY_FILE=localhost+2-key.pem yarn start

Note please replace localhost+2.pem and localhost+2-key.pem with the files you will get from mkcert.

Screenshot from 2020-10-16 01-15-28
Screenshot from 2020-10-16 01-15-43
Screenshot from 2020-10-16 01-00-55

Hey @slorber @lex111 can you Please review these changes, Thanks !

Ref: #3445

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 15, 2020
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

@ThakurKarthik could you please add some docs about using this feature?

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Oct 15, 2020

Deploy preview for docusaurus-2 ready!

Built with commit b7d88ed

https://deploy-preview-3594--docusaurus-2.netlify.app

@ThakurKarthik
Copy link
Contributor Author

Sure @lex111 , you want me to document this in the website right? I will add that !

@lex111
Copy link
Contributor

lex111 commented Oct 15, 2020

@ThakurKarthik yes please, and resolve conflict too.

@slorber
Copy link
Collaborator

slorber commented Oct 16, 2020

LGTM

Yeah basically we need the docs to know how to run Docusaurus on https locally so that our users know how to do it too. Basically it's adding your test plans to the docs somewhere (maybe on the start command doc?)

Need to resolve the merge conflict

I know from source issue (#3445) that CRA use env variables. Wonder how they made the choice of env variables VS cli options. Any idea?

@slorber slorber linked an issue Oct 16, 2020 that may be closed by this pull request
@ThakurKarthik
Copy link
Contributor Author

Hey @slorber I will add the steps to enable https in docs soon, as for your question of choice between env vs cli options, I am not that sure I tracked the original implementation in CRA facebook/create-react-app#5845 , sadly i can't find the reasoning behind the choice and here is a comment about moving env vars to a config file facebook/create-react-app#5845 (comment)

@slorber slorber changed the title fix: add support for key,cert in https fix(v2): add support for key,cert in https Oct 16, 2020
@slorber slorber changed the title fix(v2): add support for key,cert in https feat(v2): add support for key,cert in https Oct 16, 2020
@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Oct 16, 2020
@slorber
Copy link
Collaborator

slorber commented Oct 16, 2020

Thanks @ThakurKarthik

Hi @andriijas or @alexbrazier, wonder if you can give us some insights regarding this feature?
How did you choose between env variables, cli options or config file?

@slorber
Copy link
Collaborator

slorber commented Oct 22, 2020

@ThakurKarthik do you want me to complete the PR, and write some docs? We can keep the env variables for now, that should be fine (HTTPS is already an env variable anyway)

@ThakurKarthik
Copy link
Contributor Author

Hey @slorber sorry for delay,I can update them by today evening if you won't mind.
Thanks !

@slorber
Copy link
Collaborator

slorber commented Oct 22, 2020

sure, no hurry, just wanted to be sure this pr won't become stale :)

@ThakurKarthik
Copy link
Contributor Author

Hey @slorber @lex111 I have added docs for using https, please review it and feel free to suggest any changes !

@alexbrazier
Copy link

Thanks @ThakurKarthik

Hi @andriijas or @alexbrazier, wonder if you can give us some insights regarding this feature?
How did you choose between env variables, cli options or config file?

Hi @slorber, there wasn't really much of a discussion around the decision, it just happened to be how we had setup our fork of cra and it had been working well for our team for over 2 years. I think part of the original decision would have been because HTTPS=true was also set using an env var so it made sense to set all of the https config via an env var.

@slorber
Copy link
Collaborator

slorber commented Oct 26, 2020

Thanks, LGTM, just made the docs simpler.

Thanks @alexbrazier , as we already have env.HTTPS I guess we should do the same :)

@slorber slorber merged commit 9288443 into facebook:master Oct 26, 2020
@lex111 lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2: Add support for key, cert, ca when using https
6 participants