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

[feature]: Add option to use custom https certificates with buildpack #2946

Merged

Conversation

arturtreiberg
Copy link
Contributor

Description

Added two extra env variable definitions for custom https certificate key and cert paths. Also updated buildpack serve function to add https if custom cert paths are present in the configuration

Related Issue

Closes #2945

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. Add .env variable CUSTOM_HTTPS_KEY with absolute file path to the key file
  2. Add .env variable CUSTOM_HTTPS_CERT with absolute file path to the cert file
  3. Run buildpack serve .

Checklist

  • I have added tests to cover my changes, if necessary.
  • I have added translations for new strings, if necessary.
  • I have updated the documentation accordingly, if necessary.

@arturtreiberg arturtreiberg changed the title #2945 Option to add custom https Option to add custom https Jan 14, 2021
@arturtreiberg arturtreiberg changed the title Option to add custom https [feature]: Add option to use custom https certificates with buildpack Jan 14, 2021
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Jan 14, 2021

Messages
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against a0fdc3e

arturtreiberg added 2 commits January 14, 2021 20:37
@arturtreiberg arturtreiberg reopened this Jan 14, 2021
@jimbo jimbo added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jan 16, 2021
Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

@arturtreiberg Hello, thanks for your contribution! Seems like a natural addition.

How can we test this? Would like to verify that it works. A couple of unit tests would also be a good idea; let me know if I can help there.

@arturtreiberg
Copy link
Contributor Author

Hey @jimbo, sorry for such a late answer.

So testing wise, just adding these .env variables as described in the verification steps above does the trick. After the variables are added, by running yarn buildpack serve . does the trick and the https upward server will be started. Logger message should also say that custom certificates are provided.

Unit test wise, I would have to investigate it a little to see how can this be tested and I'm not sure when I will have the time for that :/ but I'm gonna take a look at what I can do

sirugh
sirugh previously requested changes Jan 25, 2021
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

I think this is really close - just a small addition requested. Also, if you're able to add some tests that would be very helpful.

Thanks for your contribution!

packages/pwa-buildpack/lib/Utilities/serve.js Show resolved Hide resolved
@arturtreiberg
Copy link
Contributor Author

I think this is really close - just a small addition requested. Also, if you're able to add some tests that would be very helpful.

Thanks for your contribution!

Copy link
Contributor

@jimbo jimbo left a comment

Choose a reason for hiding this comment

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

Great work, and thanks for adding tests. This works for me, recommend moving to QA. 👍

@dpatil-magento
Copy link
Contributor

QA Approved

@dpatil-magento dpatil-magento merged commit bb463ef into magento:develop Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:pwa-buildpack Progress: done version: Minor This changeset includes functionality added in a backwards compatible manner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature]: Add option to use custom https certificates with buildpack
5 participants