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

Change KMSKeyInfo.CreatedAt to a time.Time #3421

Closed
wants to merge 2 commits into from

Conversation

marktheunissen
Copy link
Contributor

@marktheunissen marktheunissen commented Aug 8, 2024

@marktheunissen
Copy link
Contributor Author

Don't know why this JS build is failing in the CI...

@ramondeklein
Copy link
Collaborator

We had the same failure in EOS console since yesterday, so looks like an external issue.

go.mod Outdated Show resolved Hide resolved
@ramondeklein
Copy link
Collaborator

This should fix the Javascript action: #3423

cesnietor
cesnietor previously approved these changes Aug 16, 2024
@marktheunissen
Copy link
Contributor Author

marktheunissen commented Aug 16, 2024

@cesnietor govet complained about errorsApi.ServeError(w, req, errorsApi.New(http.StatusUnauthorized, err.Error())) which was not something I touched in the PR.

I have just pushed a commit to change that to errorsApi.ServeError(w, req, errorsApi.New(http.StatusUnauthorized, "%s", err.Error())) which will probably fix the linting, but ultimately doesn't do anything, as the .New() call handled it fine before.

Another option is to change the linter configuration, let me know.

@ramondeklein
Copy link
Collaborator

@marktheunissen We've seen this in more repositories. It looks like the most recent golangci-lint version is more strict. We have issued PRs for both MinIO and mc already. Note that this is in fact a proper warning that needs fixing.

@ramondeklein
Copy link
Collaborator

I have just pushed a commit to change that to errorsApi.ServeError(w, req, errorsApi.New(http.StatusUnauthorized, "%s", err.Error())) which will probably fix the linting, but ultimately doesn't do anything, as the .New() call handled it fine before.

I think it warns, because there could be some unexpected behavior when the error message contains formatting instructions, such as %1000d or some other extreme stuff. Not sure if it could cause a crash when %s is used without passing any arguments...

@harshavardhana
Copy link
Member

Please look at a more comprehensive update @marktheunissen at #3425

@marktheunissen
Copy link
Contributor Author

@ramondeklein I don't disagree.... :) generally speaking, the vetting is correct.

My comment was just about this specific case, where the function being called is errorsApi.New() which is:

// New creates a new API error with a code and a message
func New(code int32, message string, args ...interface{}) Error {
	if len(args) > 0 {
		return &apiError{
			code:    code,
			message: fmt.Sprintf(message, args...),
		}
	}
	return &apiError{
		code:    code,
		message: message,
	}
}

My comment was " the .New() call handled it fine before.". What I meant is that passing 0 args bypasses the call to fmt.Sprintf, but the linter doesn't know that. Edge case I suppose

@harshavardhana has completed this work in the PR #3425 so I'll close this, thanks!

@marktheunissen marktheunissen deleted the timetype branch August 20, 2024 00:48
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.

4 participants