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.

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

Remove request cert in favor https options #2618

Merged
merged 4 commits into from
Jul 21, 2020
Merged

Remove request cert in favor https options #2618

merged 4 commits into from
Jul 21, 2020

Conversation

alexander-akait
Copy link
Member

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Yes

Motivation / Use-Case

Avoid unnecessary option

Breaking Changes

Yes

Additional Info

No

@codecov
Copy link

codecov bot commented May 19, 2020 •
edited
Loading

Codecov Report

Merging #2618 into v4 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##               v4    #2618   +/-   ##
=======================================
  Coverage   92.51%   92.51%           
=======================================
  Files          37       37           
  Lines        1310     1310           
  Branches      354      354           
=======================================
  Hits         1212     1212           
  Misses         93       93           
  Partials        5        5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 022f3de...b6f595a. Read the comment docs.

@alexander-akait alexander-akait changed the base branch from master to v4 May 19, 2020 14:20
hiroppy
hiroppy requested changes May 21, 2020
View reviewed changes
lib/Server.js Outdated
@@ -573,6 +573,8 @@ class Server {

this.options.https.key = this.options.https.key || fakeCert;
this.options.https.cert = this.options.https.cert || fakeCert;

console.log(this.options.https);
Copy link
Member

Choose a reason for hiding this comment

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

Need to delete this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

hiroppy
hiroppy previously approved these changes May 21, 2020
View reviewed changes
@hiroppy hiroppy dismissed their stale review May 21, 2020 11:06

sorry, my mistake...

alexander-akait
alexander-akait commented Jul 21, 2020
View reviewed changes
Copy link
Member Author

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

/cc @hiroppy @Loonride

@alexander-akait
refactor: remove the requestCert option
7c620fa 
BREAKING CHANGE: the `requestCert` was removed in favor `https` options
@alexander-akait
refactor: fix code
765097b
@alexander-akait
test: fix
fc6448f
@alexander-akait alexander-akait force-pushed the remove-requestCert-in-favor-https-options branch from 685e0ff to fc6448f Compare July 21, 2020 14:26
hiroppy
hiroppy reviewed Jul 21, 2020
View reviewed changes
lib/options.json Outdated
@@ -296,8 +296,8 @@
"public": {
"type": "string"
},
"requestCert": {
"type": "boolean"
"publicPath": {
Copy link
Member

Choose a reason for hiding this comment

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

why does this pr have publicPath changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@hiroppy merge problem, fixed

hiroppy
hiroppy requested changes Jul 21, 2020
View reviewed changes
Copy link
Member

@hiroppy hiroppy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

we should add requestCert to this line because https has it.
https://github.com/webpack/webpack-dev-server/blob/v4/lib/Server.js#L551

FYI: https://github.com/webpack/webpack-dev-server/blob/v4/lib/options.json#L132

@alexander-akait
Copy link
Member Author

alexander-akait commented Jul 21, 2020 •
edited
Loading

@hiroppy no, we already have this.options.https.requestCert, it is boolean value, so we don't need to read it from filesystem, just add console.log to check it, no all properties can be file or buffer

@hiroppy
Copy link
Member

hiroppy commented Jul 21, 2020 •
edited
Loading

oh, sorry my for misreading.

@alexander-akait alexander-akait merged commit f27bbb1 into v4 Jul 21, 2020
@alexander-akait alexander-akait deleted the remove-requestCert-in-favor-https-options branch July 21, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wizardofhogwarts wizardofhogwarts wizardofhogwarts approved these changes

@hiroppy hiroppy hiroppy approved these changes

@knagaitsev knagaitsev Awaiting requested review from knagaitsev

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

Successfully merging this pull request may close these issues.
3 participants
@alexander-akait @hiroppy @wizardofhogwarts