-
Notifications
You must be signed in to change notification settings - Fork 22
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: update go linting + update dependencies for controller #137
chore: update go linting + update dependencies for controller #137
Conversation
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
Signed-off-by: Mathew Wicks <5735406+thesuperzapper@users.noreply.github.com>
@ederign can you please LGTM when you are ready, this is an important update, and we need to get other PRs to rebase on it so they get the new linting. /approve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thesuperzapper thank you so much for this important PR. Just want to double check a few things before +1.
@@ -96,7 +96,6 @@ var _ = Describe("WorkspaceKind Webhook", func() { | |||
} | |||
|
|||
for _, tc := range testCases { | |||
tc := tc // Create a new instance of tc to avoid capturing the loop variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thesuperzapper I want to double-check with you that this doesn't break anything. Based on the comment, the variable looks pretty intentional :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's no longer necessary to copy variables in the loop because go 1.22 fixed the issue.
It's actually a thing required by the linter now, because it would be wasteful to copy it twice.
@@ -4,37 +4,11 @@ kind: ClusterRole | |||
metadata: | |||
name: manager-role | |||
rules: | |||
- apiGroups: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, are sure about changing the roles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(just to verify that this is not something automatically added).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is automatically generated, I think it just re-ordered them.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ederign, thesuperzapper The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR makes the following changes:
Updates to dependencies used on the controller:
Updates to dependencies used by backend:
Improved the linting for all go projects (controller, backend):