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

fix: use a lockfile with the runtime dependency install step #633

Merged
merged 6 commits into from May 25, 2021

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented May 25, 2021

Details

This PR updates our runtime dependency install step to use yarn install --frozen-lockfile with the same lockfile we use for builds.

Using --ignore-engines isn't really ideal, but it's consistent with the old behavior and we probably don't want to force an old node version as long as we're using a composite action and doing so would pollute the calling environment.

--ignore-optional is required to avoid double-installing puppeteer (apify has an optionalDependency on Puppeteer 5.3.0; yarn's default behavior is to install that in addition to our explicit 5.5.0 dependency)

Performance seems to be similar between the old strategy and the new:

Motivation

addresses #632

Context

This is a draft while we validate the perf impact; locally, this is much slower than npm install

Pull request checklist

  • Addresses an existing issue: addresses Action does not pin the versions of dependencies installed at runtime #632
  • [n/a] Added relevant unit test for your changes. (yarn test)
  • [n/a] Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • Ran precheckin (yarn precheckin)
  • (after PR created) The Accessibility Checks (pull_request) check should fail. All other checks should pass.

@codecov-commenter
Copy link

Codecov Report

Merging #633 (ae6ce75) into main (4f43a9a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #633   +/-   ##
=======================================
  Coverage   94.81%   94.81%           
=======================================
  Files          25       25           
  Lines         559      559           
  Branches       54       54           
=======================================
  Hits          530      530           
  Misses         29       29           

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 4f43a9a...ae6ce75. Read the comment docs.

@github-actions
Copy link
Contributor

Accessibility Insights Accessibility Insights Action

  • URLs: 1 URL(s) failed, 3 URL(s) passed, and 0 were not scannable
  • Rules: 5 check(s) failed, 11 check(s) passed, and 39 were not applicable
  • Download the Accessibility Insights artifact to view the detailed results of these checks

Failed instances

  • 3 × label: Ensures every form element has a label
  • 1 × html-has-lang: Ensures every HTML document has a lang attribute
  • 1 × image-alt: Ensures elements have alternate text or a role of none or presentation

This scan used axe-core 4.2.1 with Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.75 Safari/537.36.

@dbjorge dbjorge marked this pull request as ready for review May 25, 2021 23:05
@dbjorge dbjorge requested a review from a team as a code owner May 25, 2021 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants