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

Chore: Replace go-multierror with errors package #66432

Merged
merged 4 commits into from
Jun 19, 2023

Conversation

iSatVeerSingh
Copy link
Contributor

What is this feature?

Replaced go-multierror package with errors package.

Why do we need this feature?

Who is this feature for?

Which issue(s) does this PR fix?:

Fixes #63686

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@iSatVeerSingh iSatVeerSingh requested review from a team as code owners April 13, 2023 08:01
@iSatVeerSingh iSatVeerSingh requested a review from a team April 13, 2023 08:01
@iSatVeerSingh iSatVeerSingh requested a review from a team as a code owner April 13, 2023 08:01
@iSatVeerSingh iSatVeerSingh requested review from vtorosyan, IevaVasiljeva, sakjur, papagian and zserge and removed request for a team April 13, 2023 08:01
@grafanabot grafanabot added area/backend pr/external This PR is from external contributor labels Apr 13, 2023
@armandgrillet armandgrillet added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels May 5, 2023
@armandgrillet armandgrillet added this to the 10.0.x milestone May 8, 2023
Copy link
Contributor

@papagian papagian left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I think that, now, after upgrading the target Go version to 1.20 we can merge it.
it would be nice to update also the go.mod (by syncing your branch with the latest main and run go mod tidy) so that the dependency becomes indirect.

@papagian
Copy link
Contributor

papagian commented May 24, 2023

@iSatVeerSingh have you closed it by accident? I think that we a few additional changes we could have merged it
shall I re-open it?

@iSatVeerSingh iSatVeerSingh reopened this May 24, 2023
@papagian
Copy link
Contributor

papagian commented Jun 2, 2023

@iSatVeerSingh can you try to follow the instructions and resolve the conflict: the pkg/codegen/diffwrite.go has been deleted from the main branch therefore you need to remove it also from your branch
(I don't have permission to push to your branch to update it myself)

also can you please run go mod tidy and push the change in the go.mod file as well, thank you!

@iSatVeerSingh iSatVeerSingh force-pushed the fix/replace-go-multierror branch 2 times, most recently from b0a5653 to 3791574 Compare June 2, 2023 16:30
@iSatVeerSingh
Copy link
Contributor Author

@papagian I run go mod tidy. It is adding //indirect instead of removing package

@grobinson-grafana
Copy link
Contributor

Hi! 👋 You have some lint errors that are causing the build to fail:

pkg/services/ngalert/state/cache_test.go:69:3: S1021: should merge variable declaration with assignment on next line (gosimple)
		var multierr []error
		^
pkg/services/ngalert/state/cache_test.go:105:3: S1021: should merge variable declaration with assignment on next line (gosimple)
		var multierr []error
		^
pkg/services/ngalert/state/cache_test.go:71:19: errorsas: second argument to errors.As must be a non-nil pointer to either a type that implements error, or to any interface type (govet)
		require.True(t, errors.As(err, &multierr))
		                ^
pkg/services/ngalert/state/cache_test.go:107:19: errorsas: second argument to errors.As must be a non-nil pointer to either a type that implements error, or to any interface type (govet)
		require.True(t, errors.As(err, &multierr))

@papagian
Copy link
Contributor

papagian commented Jun 9, 2023

@papagian I run go mod tidy. It is adding //indirect instead of removing package

I think, that's fine; it's because another library we import uses it:

go mod why github.com/hashicorp/go-multierror
# github.com/hashicorp/go-multierror
github.com/grafana/grafana/pkg/codegen
github.com/grafana/codejen
github.com/hashicorp/go-multierror

Copy link
Contributor

@papagian papagian left a comment

Choose a reason for hiding this comment

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

I left some suggestions for fixing the tests

pkg/services/ngalert/state/cache_test.go Outdated Show resolved Hide resolved
pkg/services/ngalert/state/cache_test.go Outdated Show resolved Hide resolved
pkg/services/ngalert/state/cache_test.go Outdated Show resolved Hide resolved
@iSatVeerSingh iSatVeerSingh reopened this Jun 9, 2023
@iSatVeerSingh
Copy link
Contributor Author

iSatVeerSingh commented Jun 9, 2023

@papagian Added some changes. All the backend tests are passing on my local system.

@iSatVeerSingh
Copy link
Contributor Author

iSatVeerSingh commented Jun 9, 2023

I don't know why it is getting lint errors.

Copy link
Contributor

@papagian papagian left a comment

Choose a reason for hiding this comment

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

regarding the lint failures I think it's fine to ignore them for the tests

I cannot reproduce the memcached integration test failure locally and I think it's not related to the changes (must be a flaky test): let's give it a 2nd try

var multierr *multierror.Error
require.True(t, errors.As(err, &multierr))
require.Equal(t, multierr.Len(), 2)
multierr, is := err.(interface{ Unwrap() []error })
Copy link
Contributor

@papagian papagian Jun 16, 2023

Choose a reason for hiding this comment

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

I think we hit a linter limitation here: in any case I think that it's fine to ignore the linter

Suggested change
multierr, is := err.(interface{ Unwrap() []error })
//nolint:errorlint
multierr, is := err.(interface{ Unwrap() []error })

var multierr *multierror.Error
require.True(t, errors.As(err, &multierr))
require.Equal(t, multierr.Len(), 1)
multierr, is := err.(interface{ Unwrap() []error })
Copy link
Contributor

@papagian papagian Jun 16, 2023

Choose a reason for hiding this comment

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

same as above

Suggested change
multierr, is := err.(interface{ Unwrap() []error })
//nolint:errorlint
multierr, is := err.(interface{ Unwrap() []error })

Copy link
Contributor

@papagian papagian left a comment

Choose a reason for hiding this comment

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

thank you for your contribution!

@papagian papagian modified the milestones: 10.0.x, 10.1.x Jun 19, 2023
@papagian papagian added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Jun 19, 2023
@papagian papagian changed the title Build: Replace go-multierror with errors package Chore: Replace go-multierror with errors package Jun 19, 2023
@papagian papagian merged commit 1bfa3a0 into grafana:main Jun 19, 2023
13 checks passed
@iSatVeerSingh iSatVeerSingh deleted the fix/replace-go-multierror branch June 20, 2023 09:39
LudoVio pushed a commit that referenced this pull request Jun 26, 2023
* code refactor and type assertions added to tests

* no-lint rule added for specific line
harisrozajac pushed a commit that referenced this pull request Jun 30, 2023
* code refactor and type assertions added to tests

* no-lint rule added for specific line
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace go-multierror with errors package in Go 1.20
6 participants