Skip to content

Fixed errcheck issues in server/channels/app/import_functions_test.go #29229

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

Jump to bottom
Merged
hanzei merged 1 commit into from
Nov 21, 2024
Merged

Fixed errcheck issues in server/channels/app/import_functions_test.go #29229

hanzei merged 1 commit into from
Nov 21, 2024

Conversation

@edlerd
Copy link
Contributor

@edlerd edlerd commented Nov 12, 2024

Summary

Fixed errcheck issues in server/channels/app/import_functions_test.go

Ticket Link

Fixes #28949

@mm-cloud-bot
Copy link

mm-cloud-bot commented Nov 12, 2024

@edlerd: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

I understand the commands that are listed here

@mattermost-build
Copy link
Contributor

mattermost-build commented Nov 12, 2024

Hello @edlerd,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@hanzei hanzei self-requested a review November 13, 2024 06:14
@hanzei hanzei added the 2: Dev Review Requires review by a developer label Nov 13, 2024
@hanzei
Copy link
Contributor

hanzei commented Nov 13, 2024

/release-note-none

@mm-cloud-bot mm-cloud-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed labels Nov 13, 2024
hanzei
hanzei requested changes Nov 13, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Nice PR 👍

There are some linter errors to clean up:

Error: channels/app/import_functions_test.go:74:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:83:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:92:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:101:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:158:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:214:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:262:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:271:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:280:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:289:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:346:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:402:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:626:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:643:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:652:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:660:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:668:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:677:2: calling require.NotNil on error, please use require.Error instead
Error: channels/app/import_functions_test.go:691:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:698:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:715:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:722:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:741:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:743:2: calling require.Nil on error, please use require.NoError instead
Error: channels/app/import_functions_test.go:5581:5: calling require.NoError on *github.com/mattermost/mattermost/server/public/model.AppError, please use require.Nil instead
Error: channels/app/import_functions_test.go:5589:5: calling require.NoError on *github.com/mattermost/mattermost/server/public/model.AppError, please use require.Nil instead
Error: channels/app/import_functions_test.go:5734:5: calling require.NoError on *github.com/mattermost/mattermost/server/public/model.AppError, please use require.Nil instead

Please let me know if you need any help

@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Nov 13, 2024
@edlerd edlerd force-pushed the fix-lint-import-func-test branch from 39f9008 to 017f1ba Compare November 13, 2024 13:58
@edlerd edlerd requested a review from hanzei November 13, 2024 13:59
@edlerd
Copy link
Contributor Author

edlerd commented Nov 13, 2024

Updated the linter issued and ensured make vet passes locally without issues. Maybe add it as a description to the other "Fix errcheck issues ..." descriptions. Because I ensured errcheck is ok, but didn't expect make vet to find more issues.
Please give another review.

hanzei
hanzei reviewed Nov 13, 2024
View reviewed changes
@edlerd edlerd force-pushed the fix-lint-import-func-test branch from 017f1ba to 9cb3ca9 Compare November 13, 2024 20:45
@edlerd edlerd requested a review from hanzei November 13, 2024 21:16
@edlerd
Copy link
Contributor Author

edlerd commented Nov 13, 2024

test cases and make vet passes locally ✔️

@hanzei
Copy link
Contributor

hanzei commented Nov 14, 2024

Let's see if CI is also happy

hanzei
hanzei requested changes Nov 14, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Great work @edlerd 🎉

A few comments below

@edlerd edlerd force-pushed the fix-lint-import-func-test branch from 9cb3ca9 to 4b1dbd6 Compare November 14, 2024 08:55
@edlerd edlerd requested a review from hanzei November 14, 2024 08:56
hanzei
hanzei requested changes Nov 14, 2024
View reviewed changes
@edlerd edlerd force-pushed the fix-lint-import-func-test branch from 4b1dbd6 to 7c4d730 Compare November 14, 2024 09:45
@edlerd edlerd requested a review from hanzei November 14, 2024 09:45
@hanzei
Copy link
Contributor

hanzei commented Nov 14, 2024

@edlerd Heads up that there is no need to force push to your branch. The commits get squashed any way before merging them.

@edlerd
Copy link
Contributor Author

edlerd commented Nov 14, 2024

Not sure why the enterprise tests are failing. I don't have access to the results and the details link is 404 for me. Are those failures maybe unrelated to the changes introduced here?

@hanzei
Copy link
Contributor

hanzei commented Nov 14, 2024

Your branch is out f sync with the closed source code. I've merged master into your branch. That should fix the check.

@edlerd
Copy link
Contributor Author

edlerd commented Nov 14, 2024

@hanzei the merge worked, but ci needs to be triggered again and I guess this might be ready to merge after it passes.

@hanzei
Copy link
Contributor

hanzei commented Nov 19, 2024 •
edited
Loading

Sorry, @edlerd, there is yet another merge conflict. Can you please take a look? Thanks!

@edlerd edlerd force-pushed the fix-lint-import-func-test branch from db98bf6 to be30a4d Compare November 19, 2024 13:31
@edlerd
Copy link
Contributor Author

edlerd commented Nov 19, 2024

I rebased on main and resolved the conflicts. @hanzei please run ci and merge as it passes.

@hanzei hanzei removed the Awaiting Submitter Action Blocked on the author label Nov 20, 2024
hanzei
hanzei approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Great work! There are some unresolved comments, but none of them is blocking. e.g. https://github.com/mattermost/mattermost/pull/29229/files#r1841724285

@hanzei hanzei requested review from a team and BenCookie95 and removed request for a team November 20, 2024 09:34
@edlerd edlerd force-pushed the fix-lint-import-func-test branch from be30a4d to aa8a3f0 Compare November 20, 2024 09:54
@edlerd
Copy link
Contributor Author

edlerd commented Nov 20, 2024

Great work! There are some unresolved comments, but none of them is blocking. e.g. https://github.com/mattermost/mattermost/pull/29229/files#r1841724285

Woops, those probably surfaced again from a bad merge.

Addressed all the issue now, please check it works all fine by now @hanzei

hanzei
hanzei approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Awesome 🚀

BenCookie95
BenCookie95 approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@BenCookie95 BenCookie95 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @edlerd

@edlerd
Copy link
Contributor Author

edlerd commented Nov 21, 2024

Great, this was a big one with quite a few learnings for me. Thx for the reviews and suggestions 😀

@hanzei hanzei added this to the v10.4.0 milestone Nov 21, 2024
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a developer labels Nov 21, 2024
@hanzei hanzei merged commit d59c5b2 into mattermost:master Nov 21, 2024
28 checks passed
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Nov 25, 2024
isacikgoz pushed a commit that referenced this pull request Dec 6, 2024
@edlerd @isacikgoz
Fixed errcheck issues in server/channels/app/import_functions_test.go (…
50644da 
…#29229)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BenCookie95 BenCookie95 BenCookie95 approved these changes

@hanzei hanzei hanzei approved these changes

Assignees

No one assigned

Labels

4: Reviews Complete All reviewers have approved the pull request Changelog/Not Needed Does not require a changelog entry Contributor Docs/Not Needed Does not require documentation release-note-none Denotes a PR that doesn't merit a release note.

Projects

None yet

Milestone

v10.4.0

Development

Successfully merging this pull request may close these issues.

Fix errcheck issues in server/channels/app/import_functions_test.go

6 participants

@edlerd @mm-cloud-bot @mattermost-build @hanzei @BenCookie95 @amyblais