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

[inv test] Do not run gofmt unless fail-on-fmt is set #9631

Closed
wants to merge 1 commit into from

Conversation

albertvaka
Copy link
Contributor

What does this PR do?

Do not run gofmt as part of the linting stage of the test invoke task unless we want formatting errors to fail the test (ie: --fail-on-fmt is passed in).

Motivation

I don't think it's any useful.

@albertvaka albertvaka added changelog/no-changelog [deprecated] team/agent-platform [deprecated] qa/skip-qa - use other qa/ labels [DEPRECATED] Please use qa/done or qa/no-code-change to skip creating a QA card labels Oct 22, 2021
@albertvaka albertvaka added this to the 7.33.0 milestone Oct 22, 2021
@albertvaka albertvaka requested a review from a team as a code owner October 22, 2021 18:15
mx-psi
mx-psi previously approved these changes Oct 25, 2021
@albertvaka
Copy link
Contributor Author

It seems that gofmt changing the files prevented go vet from failing with package comment is detached; there should be no blank lines between it and the package statement on some files. We can't merge this as-is.

@mx-psi
Copy link
Member

mx-psi commented Oct 25, 2021

It seems that gofmt changing the files prevented go vet from failing with package comment is detached; there should be no blank lines between it and the package statement on some files. We can't merge this as-is.

So that means there are ill-formatted files and the fmt check was not working properly even on Linux then?

@albertvaka
Copy link
Contributor Author

I think it's because go vet is confused by the CRLF line endings.

@mx-psi
Copy link
Member

mx-psi commented Oct 25, 2021

I think it's because go vet is confused by the CRLF line endings.

👍 that would make sense because e.g. this one seems correct:

/*
Package metadata provides clients for Metadata APIs exposed by the ECS agent.
There are three versions of these APIs:
- V1: also called introspection endpoint.
- V2: available since ecs-agent 1.17.0 with the EC2 launch type and since
platform version 1.1.0 with the Fargate launch type.
- V3: available since ecs-agent 1.21.0 with the EC2 launch type and since
platform version 1.3.0 with the Faragate launch type.
Each of these versions sits in its own subpackage.
*/
package metadata

at least when compared with the multi-line example here, but go vet revive is complaining about it.

@mx-psi mx-psi dismissed their stale review October 25, 2021 11:46

see above

@mx-psi
Copy link
Member

mx-psi commented Oct 25, 2021

After looking into it a bit more, my guess is that this is related to golang/go#41197 because of how revive uses it and needs a fix on revive.

@mx-psi
Copy link
Member

mx-psi commented Oct 25, 2021

This is not easy to fix, I though they added a new field (EndSlash) but instead in the end they just documented the issue (golang/go@a14e7bf). I am going to file an issue on revive since I don't have a trivial fix.

@albertvaka albertvaka force-pushed the albertvaka/test-no-gfmt branch from 3c980ef to 3bcdb67 Compare October 25, 2021 13:46
@albertvaka
Copy link
Contributor Author

albertvaka commented Oct 25, 2021

Closing since we actually need gofmt to change the line endings due to a bug in revive.

@albertvaka albertvaka closed this Oct 25, 2021
@mx-psi mx-psi deleted the albertvaka/test-no-gfmt branch December 13, 2022 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog [deprecated] qa/skip-qa - use other qa/ labels [DEPRECATED] Please use qa/done or qa/no-code-change to skip creating a QA card [deprecated] team/agent-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants