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.

Sign up for GitHub

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

go_modules: don't raise error for go mod tidy #2830

Merged
merged 6 commits into from
Dec 10, 2020
Merged

go_modules: don't raise error for go mod tidy #2830

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8460ce5
go_modules: don't raise error for go mod tidy
fatih Dec 8, 2020
e839c12
go_modules: fail silently for all cases
fatih Dec 9, 2020
a496373
go_modules: fix linter warning
fatih Dec 9, 2020
c3995ee
Update go_modules/spec/dependabot/go_modules/file_updater/go_mod_upda…
Dec 9, 2020
61b4ec5
go_modules: fix the added new test
fatih Dec 9, 2020
aa67f7d
Remove expectation of error message and add a comment
feelepxyz Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions go_modules/lib/dependabot/go_modules/file_updater/go_mod_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

command = "go mod tidy"
_, stderr, status = Open3.capture3(ENVIRONMENT, command)
handle_subprocess_error(stderr) unless status.success?

# we explicitly don't raise an error for 'go mod tidy' and silently
# 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).
Open3.capture3(ENVIRONMENT, command)
end

def run_go_vendor
Expand Down
10 changes: 8 additions & 2 deletions go_modules/spec/dependabot/go_modules/file_updater/go_mod_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,13 +225,19 @@
# OpenAPIV2 has been renamed to openapiv2 in this version
let(:dependency_version) { "v0.5.1" }

it "raises a DependencyFileNotResolvable error" do
it "does not raises a DependencyFileNotResolvable error" do
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|
Copy link
Contributor

Choose a reason for hiding this comment

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

👋 @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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @fatih. Since #2627 introduced this test, and since that PR doesn't mention anything about go mod tidy, I'd like to ask @jurre to take a look before we ship this PR.

@jurre: Given your context on #2627, should we be changing this test here, or would it make more sense to delete it entirely?

Copy link
Contributor

@feelepxyz feelepxyz Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this case is only caught by go mod tidy where the import path doesn't match the exported package name, import here:

dependabot-core/go_modules/spec/fixtures/projects/renamed_package/main.go

Line 4 in 7ce0ebf

_ "github.com/googleapis/gnostic/OpenAPIv2"

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, @feelepxyz is right 👍👍

expect(error.message).to include("googleapis/gnostic/OpenAPIv2")
end
fatih marked this conversation as resolved.
Show resolved Hide resolved
end

it "updates the go.mod" do
expect(updater.updated_go_mod_content).to include(
%(github.com/googleapis/gnostic v0.5.1 // indirect\n)
)
end
end
end

Expand Down