added develop command options to use custom key and certs

[jeffwillette]

No description provided.

[jeffwillette]

reference to #5044

[gatsbybot]

Deploy preview for using-drupal ready!

Built with commit c664a68

https://deploy-preview-5056--using-drupal.netlify.com

[gatsbybot]

Deploy preview for gatsbygram ready!

Built with commit c664a68

https://deploy-preview-5056--gatsbygram.netlify.com

[pieh]

I am just thinking if this condition should go before if (program.https).

Then if people would use gatsby develop --https --cert-file=some_path --key-file=other_path it would prioritze custom ssl "files" over generated one. Does this make sense?

[jeffwillette]

yeah it makes sense, my initial thought was that the precedence should go to https since that is the simpler and most likely to be used by most people. I can see arguments for both ways though

[pieh]

Just copy-pasting from discord conversation for visibility here:

I have no real opinion - just askin question - what should be behaviour if user use gatsby develop --https --cert-file=some_path --key-file=other_path - should it use provided cert file or should it generate one

I would think it should use provided cert file, but I won't fight over it if You think otherwise ;)

reason for this - is --https option doesn't mention that it auto generates cert (either in option name or option description) - so this feels to me like this enables https in general - and if you use other optional args (cert-file,key-file) it would use cert you provided and if you wouldn't use those then we would fallback to cert autogeneration

and then using gatsby develop --cert-file=some_path --key-file=other_path (without --https) would implicitely enable --https

[jeffwillette]

yeah I can see both ways. I'll change it to whatever it needs to be to get it merged. Just let me know

[pieh]

In general this looks good to me - only thing to iron out is what should be behaviour of using gatsby develop --https --cert-file=some_path --key-file=other_path (discussion started in code inline comment) - if this should be as it currently is in PR or should be slightly adjusted.

[jeffwillette]

@KyleAMathews do you have an opinion on which way it should be?

[m-allanson]

Should --cert-file and --key-file always be used together? It might be nice to output a warning if you use one without the other.

[jeffwillette]

as far as I know they should always be used together. I will add a warning

[m-allanson]

Thanks! Thinking about this a bit more... maybe we can pass this all over to the getSslCert util?

program.ssl = await getSslCert(program.sitePackageJson.name, program[`cert-file`], program[`key-file`])

And use the following behaviour:

• --https enables https with generated certificate
• --cert-file --key-file does nothing as there's no --https flag (maybe print a message "you should add the --https flag")
• --https --cert-file --key-file enables https with custom cert and keyfile
• --https --key-file error with a message that --cert-file is missing
• --https --cert-file error with a message that --key-file is missing

And then if you specify a non-existent path for keyfile or certfile, you can log a message to the console ~~~and fall back to using a generated certificate~~~.

Does that solve the problem of which flags take precedence?

[jeffwillette]

yeah I guess that does it, I am not familiar with the getSslCert function I didn't know it would take care of it.

Just to make sure...I should add a long if/else if chain in the command code that covers each one of those cases?

[m-allanson]

I am not familiar with the getSslCert function I didn't know it would take care of it.

Well, it doesn't yet :) But it could be a way to avoid chucking a bunch of logic into the develop command. So commands/develop.js could be something like:

  // Check if https is enabled, then create or get SSL cert.
  // Certs are named after `name` inside the project's package.json.
  if (program.https) {
    program.ssl = await getSslCert({
      name: program.sitePackageJson.name
      certFile: program[`cert-file`]
      keyFile: program[`key-file`]
      directory: program.directory
    })
  }

Then the logic of setting up the ssl cert could be handled in get-ssl-cert.js, maybe something like:

const getDevelopmentCertificate = require(`devcert-san`).default
const report = require(`gatsby-cli/lib/reporter`)

// passing in extra vars here
module.exports = async ({name, certFile, keyFile, directory}) => {
  report.info(`setting up SSL certificate (may require sudo)\n`)

  // check for certFile and keyFile options

  // warn/error if only one option is set

  // warn/error if options are set but files don't exist

  // if everything looks good, return custom https config
  // return {
  //   keyPath,
  //   certPath,
  //   key: fs.readFileSync(keyPath),
  //   cert: fs.readFileSync(certPath),
  // }

  // if we get this far then we're just using the generated https certifice..

  try {
    return await getDevelopmentCertificate(name, {
      installCertutil: true,
    })
  } catch (err) {
    report.panic(`\nFailed to generate dev SSL certificate`, err)
  }

  return false
}

That's just a suggestion, feel free to do it another way if you like!

Maybe there's no need to fall back to a generated certificate if you only provide one of the file options, just error and stop (I've updated that in my previous comment).

[jeffwillette]

oh right yeah that makes sense, I felt weird throwing it all in the command. I'll update with that soon. Thanks

[jeffwillette]

@m-allanson I took your suggestion, but I just let node throw a rejection if the files don't exist because I thought that might be the best way to go about that part.

The exact description of how the precedence works is in the latest commit message and the docs. Let me know if you need anything else

[pieh]

maybe use path.join instead of manual string concat for paths?
this also forces relative path right now so probably should be noted in docs?

[m-allanson]

Thanks for working through all this @deltaskelta! It's looking 👍, if you can address @pieh's latest comments then I think we're good to go 😅.

[jeffwillette]

@m-allanson I am working on it now

[m-allanson]

This LGTM, thanks @deltaskelta 🎉

[pieh]

I think this needs update and then this is good to go in.

[jeffwillette]

@pieh I fixed what you mentioned and now the test are failing but I doubt it is from the code here...

EDIT: It looks like it was just a fluke that it failed

[pieh]

This is missing --https flag I think?

[jeffwillette]

Oh yes sorry about that

[jeffwillette]

@m-allanson or @pieh do you have any idea why the tests failed? I can't seem to make out the reason why that would happen when I only made small doc changes.

In any case, is there anything else you see that needs to be done before this can be merged? Thanks

[pieh]

Not sure why tests failed before. This right now looks good to me. Merged master and just waiting for tests to pass (hopefully) and will approve it then.

[pieh]

LGTM

[m-allanson]

This is merged, thanks again @deltaskelta.