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

Enable golangci-lint and go unit test github actions #409

Merged
merged 11 commits into from
Feb 8, 2024
Merged

Conversation

kke
Copy link
Collaborator

@kke kke commented Jan 23, 2024

  • Enables the golangci-lint action
  • Updates .golangci.yml
  • Fixes all linting errors
  • Adds a simple go.mod/go.sum consistency check
  • Enables a go unit test workflow
  • Fixes all the errors encountered in unit tests

Some of the modified error messages may be overly verbose and repetitive, but I believe there will be more context to work with when a user reports such an error.

There were a couple of linting errors that were security vulnerabilities found by gosec.

Unit tests have not been passing since Dec 2022.

Copy link
Contributor

@cranzy cranzy left a comment

Choose a reason for hiding this comment

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

Gh action looks good.

It seems to be pointing the linting issue already :)

@james-nesbitt
Copy link
Collaborator

will the issues reported get fixed? you could always leave the disabled/enabled plugin list as is, which would result in no errors.

@kke kke force-pushed the lint-workflow branch 2 times, most recently from 47a2386 to 646a2dc Compare January 26, 2024 08:48
@kke kke changed the title Enable golangci-lint github action Enable golangci-lint and go unit test github actions Jan 26, 2024
@kke
Copy link
Collaborator Author

kke commented Jan 26, 2024

As the Jenkins workflows aren't working, maybe they should be removed.

Anything that doesn't go green should not be merged.

@kke
Copy link
Collaborator Author

kke commented Jan 26, 2024

This will probably conflict @squizzi 's MSR3 PR quite bad.

@kke kke added non-breaking change Does not change functionality or require user actions security fix Security fix labels Jan 26, 2024
@kke
Copy link
Collaborator Author

kke commented Feb 5, 2024

Or squizzis PR makes this conflict but not as bad as I thought :)

@james-nesbitt
Copy link
Collaborator

I'll check out how hard this is to rebase now.

@james-nesbitt
Copy link
Collaborator

the only real conflict I see (besides the MSR3 rework) is the recent uninstallMCR refactor that @cranzy did. I will ask him to do the rebase.

@kke
Copy link
Collaborator Author

kke commented Feb 7, 2024

I did the rebase, but there are some failing unit tests. They're not passing on master either. Should not merge anything that doesn't pass.

@cranzy
Copy link
Contributor

cranzy commented Feb 7, 2024

There was a logical mistake in the manager.go file, which caused the binary to stop on the first phase that was successful.

kke and others added 7 commits February 8, 2024 10:45
Signed-off-by: Kimmo Lehto <[email protected]>
Signed-off-by: Kimmo Lehto <[email protected]>
Signed-off-by: Kimmo Lehto <[email protected]>
Signed-off-by: Kimmo Lehto <[email protected]>
Signed-off-by: Kimmo Lehto <[email protected]>
Signed-off-by: Kimmo Lehto <[email protected]>
kke added 2 commits February 8, 2024 10:55
Signed-off-by: Kimmo Lehto <[email protected]>
@kke
Copy link
Collaborator Author

kke commented Feb 8, 2024

Just a couple of notes:

  • To keep output consistent, use log.Foo("%s: foo foo", h) instead of log.Foo("%s foo foo", h)
  • To keep errors consistent, use foo: %w instead of foo => %w.
  • The "do not define sentinel errors" that comes from return fmt.Errorf("no bueno") is slightly annoying and perhaps unnecessary as this is not a library. Maybe it could be disabled.

@cranzy
Copy link
Contributor

cranzy commented Feb 8, 2024

I've confirmed that both launchpad apply and launchpad reset work as expected locally.

@cranzy cranzy merged commit c758b02 into master Feb 8, 2024
4 checks passed
@cranzy cranzy deleted the lint-workflow branch February 8, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
non-breaking change Does not change functionality or require user actions security fix Security fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants