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

update docker image #2406

Merged
merged 6 commits into from May 21, 2020
Merged

update docker image #2406

merged 6 commits into from May 21, 2020

Conversation

dpatil-magento
Copy link
Contributor

@dpatil-magento dpatil-magento commented May 19, 2020

Description

Current docker image is too old and once #2298 is merged, build and deploy jobs will fail. Updated to near latest node 12 image.

Related Issue

Closes #ISSUE_NUMBER.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. AWS build/deploy jobs should pass.
  2. on local bash docker/run-docker should work.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
    [ ] I have added tests to cover my changes, if necessary.

@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress May 19, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented May 19, 2020

Fails
🚫

No linked issue found. Please link a relevant open issue by adding the text "closes #<issue_number>" or "closes JIRA-<issue_number>" in your PR.

🚫 A version label is required. A maintainer must add one.
Messages
📖

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

📖 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).

Generated by 🚫 dangerJS against 63285e4

@devops-pwa-codebuild
Copy link
Collaborator

devops-pwa-codebuild commented May 19, 2020

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2406.pwa-venia.com : LH Performance Expected 0.85 Actual 0.52, LH Best Practices Expected 1 Actual 0.92
https://pr-2406.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.35, LH Best Practices Expected 1 Actual 0.92
https://pr-2406.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.51, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92

@dpatil-magento
Copy link
Contributor Author

Tested with #2298 changes in #2408

@fooman
Copy link
Contributor

fooman commented May 19, 2020

If #2298 breaks on node 10 shouldn't we also bump https://github.com/magento/pwa-studio/blob/develop/package.json#L89 to 12?

@dpatil-magento
Copy link
Contributor Author

dpatil-magento commented May 20, 2020

If #2298 breaks on node 10 shouldn't we also bump https://github.com/magento/pwa-studio/blob/develop/package.json#L89 to 12?

Specifically, Its node 10.14.1, which is too old. anything > 10.14.1 works.
error jest@26.0.1: The engine "node" is incompatible with this module. Expected version ">= 10.14.2". Got "10.14.1"

I also tested with 10.14.2 and it works c278552. Updating CI to node docker 12 to be more future proof.

@zetlen
Copy link
Contributor

zetlen commented May 20, 2020

@dpatil-magento We could choose to declare what point-release ranges of LTS we support. It's not realistic for us to say we support all of 10.x, or all of 12.x, but we could choose a realistic set.

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

LGTM, but I had one question about using an lts alias for easier future maintenance.

@@ -2,7 +2,7 @@
# This file is intended to be used with ./docker-compose.yml #
##############################################################

FROM node:10.14.1-alpine as build
FROM node:12.16.3-alpine as build
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it our policy to always specify an exact version when pulling the docker image? This could auto-update to the most recent LTS version if you used the alias node:lts-alpine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use exact working version to avoid any bugs in latest version might have. In this way our CI env would be more stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I can have test pr to try node:lts-alpine and track it for 10-15 days and then discuss the outcomes.

Copy link
Contributor

@zetlen zetlen left a comment

Choose a reason for hiding this comment

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

I understand that we're testing using an lts alias elsewhere. Meanwhile, LGTM!

@dpatil-magento dpatil-magento merged commit ed2647c into develop May 21, 2020
@dpatil-magento dpatil-magento deleted the dpatil/update-docker branch May 21, 2020 19:07
@m2-community-project m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants