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

Fix linter errors #302

Merged
merged 4 commits into from
Dec 13, 2023
Merged

Fix linter errors #302

merged 4 commits into from
Dec 13, 2023

Conversation

mike-gregory-ovo
Copy link
Contributor

@mike-gregory-ovo mike-gregory-ovo commented Dec 13, 2023

The latest version of golangci-lint makes some critical changes to the linters, so I've fixed the various errors across multiple files.

Copy link

github-actions bot commented Dec 13, 2023

Test Results

196 tests   196 ✔️  10s ⏱️
  14 suites      0 💤
    1 files        0

Results for commit b3f330c.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@jlucktay jlucktay left a comment

Choose a reason for hiding this comment

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

I'm concerned about secrets being leaked through errors and logs and the like.
Apart from that, though, overall good refactor. 👍

@mike-gregory-ovo
Copy link
Contributor Author

Hey @jlucktay, those are the names of the expected configuration key e.g. SlackAPIKey is https://github.com/ovotech/go-sync/blob/fix-linter-errors/adapters/slack/usergroup/usergroup.go#L56-L59. The ErrorContains is just to ensure that the error contains the name of the key that's failing.

@jlucktay
Copy link
Contributor

Hey @jlucktay, those are the names of the expected configuration key e.g. SlackAPIKey is https://github.com/ovotech/go-sync/blob/fix-linter-errors/adapters/slack/usergroup/usergroup.go#L56-L59. The ErrorContains is just to ensure that the error contains the name of the key that's failing.

OK, good to know. Those variable names are really alarming in isolation. Even that comment makes me think twice:

SlackAPIKey is an API key for authenticating with Slack.

To me, that reads like it's describing the secret itself, not the key to look up the secret in another thing somewhere else.

@mike-gregory-ovo
Copy link
Contributor Author

Agreed, bad naming on my behalf. I'm going to leave as-is for now as I imagine this will all change when we move to goplugins anyway!

@mike-gregory-ovo mike-gregory-ovo merged commit c064d94 into main Dec 13, 2023
4 checks passed
@mike-gregory-ovo mike-gregory-ovo deleted the fix-linter-errors branch December 13, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants