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 ineffassign #6102

Merged
merged 3 commits into from
Apr 11, 2023
Merged

enable ineffassign #6102

merged 3 commits into from
Apr 11, 2023

Conversation

torcolvin
Copy link
Collaborator

@torcolvin torcolvin commented Feb 21, 2023

Add ineffassign to required linters. Nearly all changes are test changes except:

  • parseSequenceInt now returns empty structs on error
  • TimedSetFromString returns error if vbucket is not an int
  • one graphql rest change to handle error
  • handle error case in rest tester if we can not get a user

The other non test changes are just removing unused variables.

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

@torcolvin torcolvin force-pushed the torcolvin/ineffassign branch 3 times, most recently from 3f9a97c to 32c2fee Compare March 2, 2023 14:10
@torcolvin torcolvin force-pushed the torcolvin/ineffassign branch from 32c2fee to a908616 Compare March 2, 2023 14:10
@@ -325,12 +325,12 @@ func (op *OIDCProvider) initOIDCClient(ctx context.Context) error {
return err
}

metadata, verifier, err := op.DiscoverConfig(ctx)
metadata, _, err := op.DiscoverConfig(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like this leaves any consumers for the verifier returned by DiscoverConfig. I'm not convinced this was an intentional change. I realize this PR doesn't change that, but I feel this might merit a tracking ticket to evaluate whether we should be using the verifier returned by DiscoverConfig in the case where we're using standard discovery.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adamcfraser adamcfraser merged commit 8903f60 into master Apr 11, 2023
@adamcfraser adamcfraser deleted the torcolvin/ineffassign branch April 11, 2023 17:50
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