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

Yarn Berry: run git lfs pull before running yarn #5833

Merged
merged 11 commits into from
Nov 1, 2022
4 changes: 4 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ RUN apt-get update \
build-essential \
dirmngr \
git \
git-lfs \
bzr \
mercurial \
gnupg2 \
Expand Down Expand Up @@ -316,6 +317,9 @@ RUN curl -sL $SHIM -o git-shim.tar.gz && mkdir -p ~/bin && tar -xvf git-shim.tar
ENV PATH="$HOME/bin:$PATH"
# Configure cargo to use git CLI so the above takes effect
RUN mkdir -p ~/.cargo && printf "[net]\ngit-fetch-with-cli = true\n" >> ~/.cargo/config.toml
# Disable automatic pulling of files stored with Git LFS
jtbandes marked this conversation as resolved.
Show resolved Hide resolved
# This avoids downloading large files not necessary for the dependabot scripts
ENV GIT_LFS_SKIP_SMUDGE=1

# Pin to an earlier version of Hex. This must be run as dependabot
RUN mix hex.install 1.0.1
18 changes: 18 additions & 0 deletions npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,24 @@ def self.required_files_message
"Repo must contain a package.json."
end

# Overridden to pull any yarn data or plugins which may be stored with Git LFS.
def clone_repo_contents
return @git_lfs_cloned_repo_contents_path if defined?(@git_lfs_cloned_repo_contents_path)

@git_lfs_cloned_repo_contents_path = super
begin
SharedHelpers.with_git_configured(credentials: credentials) do
Dir.chdir(@git_lfs_cloned_repo_contents_path) do
cache_dir = Helpers.fetch_yarnrc_yml_value("cacheFolder", "./yarn/cache")
SharedHelpers.run_shell_command("git lfs pull --include .yarn,#{cache_dir}")
end
@git_lfs_cloned_repo_contents_path
end
rescue StandardError
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to return the path and carry on for any error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other options do you have in mind? I’m not sure what meaningful recovery could be done here…

Copy link
Member

Choose a reason for hiding this comment

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

Let me first better understand, which errors are you anticipating?

Here are the ones I can think of:

  • Dependabot::SharedHelpers::HelperSubprocessFailed raised by run_shell_command in super or here
  • NoMethodError if .yarnrc.yml is empty, in which case YAML.load_file will return false, which does not respond to fetch
  • Psych::ParseError if the YAML in the .yarnrc.yml is invalid

If we encounter an exception outside of these, I think we'd want to let it fail? @jurre might know better 😅

Copy link
Member

@jurre jurre Oct 27, 2022

Choose a reason for hiding this comment

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

I think it's reasonable to continue without lfs pull in those cases rather than fail the update entirely?

You do raise a few good error cases (the empty or invalid .yarnrc.yml for example) that we should probably handle?

Copy link
Member

Choose a reason for hiding this comment

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

Related:

It took a bit to track down why this started failing on main since the error gets swallowed... that means users who are hitting this failure scenario won't be told why...

@git_lfs_cloned_repo_contents_path
end
end

private

def fetch_files
Expand Down
12 changes: 2 additions & 10 deletions npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,21 +73,13 @@ def updated_dependency_files
def vendor_cache_dir
return @vendor_cache_dir if defined?(@vendor_cache_dir)

@vendor_cache_dir = if File.exist?(".yarnrc.yml")
YAML.load_file(".yarnrc.yml").fetch("cacheFolder", "./.yarn/cache")
else
"./.yarn/cache"
end
@vendor_cache_dir = Helpers.fetch_yarnrc_yml_value("cacheFolder", "./.yarn/cache")
end

def install_state_path
return @install_state_path if defined?(@install_state_path)

@install_state_path = if File.exist?(".yarnrc.yml")
YAML.load_file(".yarnrc.yml").fetch("installStatePath", "./.yarn/install-state.gz")
else
"./.yarn/install-state.gz"
end
@install_state_path = Helpers.fetch_yarnrc_yml_value("installStatePath", "./.yarn/install-state.gz")
end

def vendor_updater
Expand Down
8 changes: 8 additions & 0 deletions npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ def self.npm_version_numeric(lockfile_content)
6
end

def self.fetch_yarnrc_yml_value(key, default_value)
if File.exist?(".yarnrc.yml") && (yarnrc = YAML.load_file(".yarnrc.yml"))
yarnrc.fetch(key, default_value)
else
default_value
end
end

# Run any number of yarn commands while ensuring that `enableScripts` is
# set to false. Yarn commands should _not_ be ran outside of this helper
# to ensure that postinstall scripts are never executed, as they could
Expand Down
35 changes: 35 additions & 0 deletions npm_and_yarn/spec/dependabot/npm_and_yarn/file_fetcher_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,41 @@
end
end

context "with .yarn data stored in git-lfs" do
let(:source) do
Dependabot::Source.new(
provider: "github",
repo: repo,
directory: directory
)
end
let(:url) { "https://api.github.com/repos/dependabot-fixtures/dependabot-yarn-lfs-fixture/contents/" }
let(:repo) { "dependabot-fixtures/dependabot-yarn-lfs-fixture" }
let(:repo_contents_path) { Dir.mktmpdir }
after { FileUtils.rm_rf(repo_contents_path) }

let(:file_fetcher_instance) do
described_class.new(source: source, credentials: credentials, repo_contents_path: repo_contents_path)
end

it "pulls files from lfs after cloning" do
# Calling #files triggers the clone
expect(file_fetcher_instance.files.map(&:name)).to contain_exactly("package.json", "yarn.lock", ".yarnrc.yml")
expect(
File.read(
File.join(repo_contents_path, ".yarn", "releases", "yarn-3.2.4.cjs")
)
).to start_with("#!/usr/bin/env node")

# LFS files not needed by dependabot are not pulled
expect(
File.read(
File.join(repo_contents_path, ".pnp.cjs")
)
).to start_with("version https://git-lfs.github.com/spec/v1")
end
end

context "with a .npmrc file" do
before do
stub_request(:get, url + "?ref=sha").
Expand Down