go_modules: don't raise error for go mod tidy #2830
Conversation
| # continue here. `go mod tidy` shouldn't block updating versions | ||
| # because there are some edge cases where it's ok to fail (such as | ||
| # generated files not available yet to us). | ||
| return if istidy |
There was a problem hiding this comment.
We still fail with Dependabot::DependencyFileNotResolvable and Dependabot::GoModulePathMismatch when running a go mod tidy. Initially I wanted to move up this line so it never fails, but it looks we have some test cases that cover these two specific error types. I know this you PR fixes one particular case, but wonder if we should also not take care of Dependabot::DependencyFileNotResolvable and Dependabot::GoModulePathMismatch ?
There was a problem hiding this comment.
We run go get (here) before we run go mod tidy. With that in mind, if there's something in the manifest that would trigger a Dependabot::DependencyFileNotResolvable error or a Dependabot::GoModulePathMismatch error, can we reasonably assume that those errors would have occurred during go get? If so, then we know that we won't even get to the go mod tidy step in that case. What do you think?
There was a problem hiding this comment.
That's a good point Jason. go get would definitely catch some of these (especially the Dependabot::DependencyFileNotResolvable errors).
| _, stderr, status = Open3.capture3(ENVIRONMENT, command) | ||
| handle_subprocess_error(stderr) unless status.success? | ||
| handle_subprocess_error(stderr, true) unless status.success? |
There was a problem hiding this comment.
It seems to me like we're trying to silently ignore any problems that occur during go mod tidy. If that's accurate, then instead of adding a new arg to handle_subprocess_error, would it make sense to skip the invocation handle_subprocess_error entirely?
| _, stderr, status = Open3.capture3(ENVIRONMENT, command) | |
| handle_subprocess_error(stderr) unless status.success? | |
| handle_subprocess_error(stderr, true) unless status.success? | |
| # Attempt command, but ignore any errors | |
| Open3.capture3(ENVIRONMENT, command) |
There was a problem hiding this comment.
This makes sense and I think we should use this idiom. Initially I changed this line and removed handling the error, but I wasn't sure about some of the edge cases and whether we should return the specific error types described above. I'll update the tests as well to accommodate the changes 👍🏼
| @@ -118,9 +118,15 @@ def update_files # rubocop:disable Metrics/AbcSize | |||
| def run_go_mod_tidy | |||
| return unless tidy? | |||
|
|
|||
| # NOTE(arslan): use `go mod tidy -e` once Go 1.16 is out: | |||
| # https://github.com/golang/go/commit/3aa09489ab3aa13a3ac78b1ff012b148ffffe367 | |||
go_modules/spec/dependabot/go_modules/file_updater/go_mod_updater_spec.rb
Outdated
Show resolved
Hide resolved
|
@fatih left a spec comment, otherwise LGTM! 🙌 The unrelated spec failures should be fixed on main now |
`go mod tidy` makes sure that go.mod matches the source code in a Go module. This also means that `dependabot` is doing a quassi linting on the code, which prevents updating versions in a go.mod if the source code doesn't got build. One particular use case is when a project is missing generated files. Sometimes people don't submit and commit the generated files of a code generator (i.e: `protobuf`) and do it in pre-script in the CI. Because `dependabot` is not aware of this pre-script, it'll run `go mod tidy` and fail for the user. We should silently continue if `go mod tidy` fails and not raise an error. This will allow dependabot still to run `go mod tidy`, but will not block users for these kind of edge cases.
…ter_spec.rb Co-authored-by: Philip Harrison <philip@mailharrison.com>
fatih
force-pushed
the
fatih/silently-fail-go-mod-tidy
branch
from
4998941 to
c3995ee
Compare
| error_class = Dependabot::DependencyFileNotResolvable | ||
| expect { updater.updated_go_sum_content }. | ||
| to raise_error(error_class) do |error| | ||
| to_not raise_error(error_class) do |error| |
There was a problem hiding this comment.
👋 @fatih: Can you help me understand why this behavior is changing in this pull request? Since we're only changing code related to go mod tidy, it's not yet clear to me why this test needs to change.
There was a problem hiding this comment.
This particular change was caused previously by go mod tidy. Because we don't return an error anymore, this test fails as it expects an error. So I either had to remove it, or change it so it doesn't return an error.
There was a problem hiding this comment.
There was a problem hiding this comment.
I believe this case is only caught by go mod tidy where the import path doesn't match the exported package name, import here:
Exported as openapi_v2: https://github.com/google/gnostic/blob/ed308846fc77db7667936d27d7ac1fb278b74216/openapiv2/OpenAPIv2.go#L17
I think we can delete this test case and rely on the below updates the go.mod as this would fail if we raised a resolvability error.
go mod tidymakes sure that go.mod matches the source code in a Gomodule. This also means that
dependabotis doing a quassi linting onthe code, which prevents updating versions in a go.mod if the source
code doesn't got build. One particular use case is when a project is
missing generated files. Sometimes people don't submit and commit the
generated files of a code generator (i.e:
protobuf) and do it inpre-script in the CI. Because
dependabotis not aware of thispre-script, it'll run
go mod tidyand fail for the user.We should silently continue if
go mod tidyfails and not raise anerror. This will allow dependabot still to run
go mod tidy, but willnot block users for these kind of edge cases.