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

Docker compose #859

Merged
merged 26 commits into from
Mar 19, 2019
Merged

Docker compose #859

merged 26 commits into from
Mar 19, 2019

Conversation

sharkySharks
Copy link
Contributor

@sharkySharks sharkySharks commented Feb 6, 2019

Description

This pull request adds the ability run PWA inside of a docker container with a secure https protocol.

Related Issue

Closes #734

Motivation and Context

Containerizing PWA will allow for more CI configuration in the future and help towards a more platform agnostic development environment. So far the setup script in this PR will only work on Mac, but in the future the hope is to get this working on Windows Docker as well.

How Has This Been Tested?

Manually.

To check this out, follow the instructions under the docs draft file

Proposed Labels for Change Type/Package

FEATURE
pwa-buildpack, venia-concept, pwa-devdocs

Checklist:

  • I have read the CONTRIBUTING document.
  • I have linked an issue to this PR.
  • I have indicated the change type and relevant package(s).
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes. - not sure the best testing approach for this but open to suggestions.
  • All new and existing tests passed.
  • All CI checks are green (linting, build/deploy, etc).
  • At least one core contributor has approved this PR.

@vercel
Copy link

vercel bot commented Feb 6, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@vercel vercel bot temporarily deployed to staging February 6, 2019 22:19 Inactive
docker/README.md Outdated
* add a custom domain, configured in `.env.docker`, to the host system's `/etc/hosts` file
* generate a self-signed ssl/tls certificate and trust the certificate in the system keychain
* run `docker-compose build` to build the container network
* run `docker-compose up` to start the container running PWA at the customer domain with https

This comment was marked as outdated.

docker/README.md Outdated
* run `docker-compose build` to build the container network
* run `docker-compose up` to start the container running PWA at the customer domain with https

After `docker/run-docker` is executed from the root of the repository, the default confgiuration will have the PWA application running at `https://pwa-docker.local`.

This comment was marked as resolved.

@@ -1,11 +1,7 @@
{

This comment was marked as resolved.

@@ -5,5 +5,6 @@
"node": "10.x.x"
},
"public": true,
"version": 1
"version": 1,
"type": "npm"

This comment was marked as outdated.

This comment was marked as outdated.

to PWADevServer configuration options.
More options for this feature are described in documentation.
`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes above are from running prettier

Copy link
Contributor

Choose a reason for hiding this comment

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

That's odd...what version of Prettier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't have prettier globally on my computer so this is coming from the repo, I can change it back and try it again. Not sure why this triggered in the first place.

host: '0.0.0.0',
port: await portscanner.findAPortNotInUse(10000),
host: config.host || '0.0.0.0',
port: config.port || (await portscanner.findAPortNotInUse(10000)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe these values should just be checked against process.env values rather than messing with them in webpack.config.js

@vercel vercel bot temporarily deployed to staging February 8, 2019 20:20 Inactive
@bbatsche
Copy link
Member

bbatsche commented Feb 8, 2019

Since .local is typically used for mDNS, should we default use one of the four TLDs reserved for development and testing: https://en.wikipedia.org/wiki/Top-level_domain#Reserved_domains ?

@brendanfalkowski
Copy link
Contributor

When Google steamrolled .dev I think .test became the standard practice. I use that even though I hate it.

@sharkySharks
Copy link
Contributor Author

sharkySharks commented Feb 8, 2019

@bbatsche great point. I'm for either .localhost since that is where it is being served in the container and mapped in /etc/hosts, or possibly .test. My only hesitation about the latter is that it is not being staged for testing per say, but maybe that is what it will be used for ultimately.

@vercel vercel bot temporarily deployed to staging February 8, 2019 21:06 Inactive
@vercel vercel bot had a problem deploying to staging February 8, 2019 21:24 Failure
@vercel vercel bot temporarily deployed to staging February 8, 2019 21:29 Inactive
@sharkySharks
Copy link
Contributor Author

well that's disconcerting, the now deploy does not fail the check if there are 500 errors from the now server...

@sharkySharks sharkySharks force-pushed the docker-compose branch 2 times, most recently from 3a9988e to 3d1bdf0 Compare February 8, 2019 21:55
@vercel vercel bot temporarily deployed to staging February 8, 2019 21:55 Inactive
docker-compose build

message "Starting Docker network and containers"
message "You may see some warnings that @magento/venia-drivers could not be \n resolved. This is normal and not an error."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is going to be resolved with #881.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed since this has been merged now

Copy link
Contributor Author

@sharkySharks sharkySharks left a comment

Choose a reason for hiding this comment

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

Comments from group review


main () {
# copy environment variables to .env in root for docker-compose to consume for build
cp ./docker/.env.docker $ENVFILE

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

-config $OpenSSLConf

# add the certificate to the system keychain and trust the certificate
sudo security add-trusted-cert -d -r trustRoot -k /Library/Keychains/System.keychain ./docker/certs/$DOMAIN.crt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

look into adding this to the login keychain rather than the system keychain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looked into adding the cert to the login keychain and while I successfully added it to the login keychain file and it was present in the keychain access gui the cert was not being trusted by the browser. despite a lot of google searching I could not get this to work in the login keychain.

Copy link
Contributor

Choose a reason for hiding this comment

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

The OSX security CLI is so weird and poorly documented that I think it's not even supposed to be public API. OH well!

@sharkySharks sharkySharks removed the hold On hold until another condition is fulfilled. label Mar 19, 2019
@supernova-at supernova-at merged commit 3f23f06 into develop Mar 19, 2019
@supernova-at supernova-at deleted the docker-compose branch March 19, 2019 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants