Raise user error when Yarn is misconfigured

[deivid-rodriguez]

Sometimes we'll fail to run yarn --version because yarn is misconfigured because it sets yarn-path inside .yanrc to a path that's not committed to the repo.

This seems like a user error, but I couldn't find a good fit for it among existing errors, so I created a new one.

Open to suggestions here.

[deivid-rodriguez]

This one is ready for feedback!

[Nishnha]

Does this mean that if a user sets their yarn-path and ships a yarn binary with their repo, we would use it? Does it require the insecure-external-code-execution flag?

I understand the reasoning behind capturing and raising this error, but what prevents us from still attempting the update? We could raise a warning about setting yarn-path and point the yarn-path` to our own yarn binary?

[deivid-rodriguez]

I think we do, yes. We delegate to yarn, and that's how yarn works. So, I don't think any flag is needed for this behavior to be like that.

In this case, user is configuring yarn-path, but the yarn-path they are configuring is not present in the repo. So yarn complains that the configuration is broken and can't run.

Yeah, we could "ignore" this error, remove yarn-path from the configuration, and try the upgrade if that makes yarn happy. It seems overkill to me though. This is a user error and I don't think it's our job to fix it?

[Nishnha]

I think we do, yes. We delegate to yarn, and that's how yarn works. So, I don't think any flag is needed for this behavior to be like that.

In this case, user is configuring yarn-path, but the yarn-path they are configuring is not present in the repo. So yarn complains that the configuration is broken and can't run.

Yeah, we could "ignore" this error, remove yarn-path from the configuration, and try the upgrade if that makes yarn happy. It seems overkill to me though. This is a user error and I don't think it's our job to fix it?

Okay gotcha, yeah if we normally do run the yarn-path binary when it's included, then I see this as a user error too!

At first I thought we should require the insecure-external-code-execution flag to use an included yarn binary, but since the user actually ships it themselves it shouldn't be an issue?

[Nishnha]

Could we add a test by raising from the SharedHelper subprocess? The code itself LGTM though.

[deivid-rodriguez]

At first I thought we should require the insecure-external-code-execution flag to use an included yarn binary, but since the user actually ships it themselves it shouldn't be an issue?

Yeah, that's my thinking too!

I'll work on a test 👍.

[Nishnha]

I notice we don't list out what the error is explicitly but that's fine since it might also change upstream. This LGTM.

[deivid-rodriguez]

Yeah!

I was actually going to test the message, in particular, because I suspect that dependabot_tmp_dir is going to show up in the user facing message and I was not sure we want that:

dependabot-core/common/lib/dependabot/errors.rb

Lines 23 to 32 in 4d22252

	def sanitize_message(message)
	return message unless message.is_a?(String)
	path_regex =
	Regexp.escape(Utils::BUMP_TMP_DIR_PATH) + "\\/" +
	Regexp.escape(Utils::BUMP_TMP_FILE_PREFIX) + "[a-zA-Z0-9-]*"
	message = message.gsub(/#{path_regex}/, "dependabot_tmp_dir").strip
	filter_sensitive_data(message)
	end

But I decided this was good enough as is, since I was seeing different behavior depending on whether I run the code through dry-run.rb, or through the specs. I'll double check next week before deploying.