TrueCloudLab/frostfs-node

Fork 27

Specify gofumpt version in the Makefile #1266

New issue

Closed

opened 2024-07-23 07:45:26 +00:00 by fyrchik · 5 comments

fyrchik commented

2024-07-23 07:45:26 +00:00

Owner

#1265 occurred, because one of the team members was using 0.6.0, while the others used the previous version.
In this task introduce fumpt-install target to install the specified version and add a separate action to tests.yml workflow.
gofumpt is already skipped by pre-commit on CI, but not used in another action, currently.

#1265 occurred, because one of the team members was using 0.6.0, while the others used the previous version. In this task introduce `fumpt-install` target to install the specified version and add a separate action to `tests.yml` workflow. `gofumpt` is already skipped by pre-commit on CI, but not used in another action, currently.

fyrchik added the

Infrastructure

label 2024-07-23 07:45:26 +00:00

elebedeva was assigned by fyrchik

2024-07-23 07:53:08 +00:00

fyrchik added this to the v0.43.0 milestone 2024-07-23 07:53:41 +00:00

fyrchik added the

internal

label 2024-07-23 07:53:49 +00:00

elebedeva referenced this issue from a pull request that will close it,

2024-08-01 11:49:26 +00:00

Specify gofumpt version in the Makefile #1286

fyrchik referenced this issue from a commit

2024-08-02 10:23:25 +00:00

[#1266] Makefile: Specify gofumpt version

fyrchik referenced this issue from a commit

2024-08-02 10:23:26 +00:00

[#1266] .forgejo: Add gofumpt action

fyrchik closed this issue

2024-08-02 10:23:30 +00:00

fyrchik reopened this issue

2024-09-12 07:58:01 +00:00

fyrchik commented

2024-09-12 07:59:36 +00:00

Author

Owner

@elebedeva #1370 fixes the gofumpt issue. It was introduced in #1362 where the action
https://git.frostfs.info/TrueCloudLab/frostfs-node/actions/runs/11191/jobs/6 succeded, even though we can see the unformatted file in its output.
Please, tell me how have you tested #1286?
I am reopening this issue to investigate.

@elebedeva #1370 fixes the gofumpt issue. It was introduced in #1362 where the action https://git.frostfs.info/TrueCloudLab/frostfs-node/actions/runs/11191/jobs/6 succeded, even though we can see the unformatted file in its output. Please, tell me how have you tested #1286? I am reopening this issue to investigate.

a-savchuk commented

2024-09-12 09:39:47 +00:00

Member

Interestingly, gofumpt always returns an exit code of 0, even when it finds misformatted files (see this issue). Currently, we list all found files, so we can check if the gofumpt output is empty with a command like this:

$ test -z "$(gofumpt -l -w pkg/)"

# another syntax for `test` command
$ [ -z "$(gofumpt -l -w pkg/)" ]

However, we not only need to check the gofumpt output, but also print it. It's getting harder🫠. What about this approach?

$ gofumpt -l -w pkg/ | tee >(cat >&2) | xargs test -z

I'm unsure how hard it will be to add this into the Makefile

Interestingly, `gofumpt` always returns an exit code of 0, even when it finds misformatted files (see [this issue](https://github.com/mvdan/gofumpt/issues/114)). Currently, we list all found files, so we can check if the `gofumpt` output is empty with a command like this: ```sh $ test -z "$(gofumpt -l -w pkg/)" # another syntax for `test` command $ [ -z "$(gofumpt -l -w pkg/)" ] ``` However, we not only need to check the `gofumpt` output, but also print it. It's getting harder🫠. What about this approach? ```sh $ gofumpt -l -w pkg/ | tee >(cat >&2) | xargs test -z ``` I'm unsure how hard it will be to add this into the Makefile

elebedeva commented

2024-09-12 12:27:15 +00:00

Member

@fyrchik I probably missed this case because of pre-commit hook failing if any files were changed and I falsely assumed workflows operate the same way. Now I see that fumt target should fail explicitly if gofumt changed any files.

@fyrchik I probably missed this case because of `pre-commit` hook failing if any files were changed and I falsely assumed workflows operate the same way. Now I see that `fumt` target should fail explicitly if `gofumt` changed any files.

fyrchik commented

2024-09-12 12:57:05 +00:00

Author

Owner

make fumpt should not fail, it should format.
But fumpt action should fail.

`make fumpt` should not fail, it should format. But `fumpt` action should fail.

👍 2

a-savchuk referenced this issue from TrueCloudLab/frostfs-api

2024-09-12 18:55:12 +00:00

.forgejo: Make 'fmt' job fail on changed files #66

a-savchuk commented

2024-09-12 19:05:56 +00:00

Member

@elebedeva I've found forgejo-runner really handy for debugging actions.

@elebedeva I've found [forgejo-runner](https://forgejo.org/docs/v1.20/user/actions/#debugging-workflows-with-forgejo-runner-exec) really handy for debugging actions.

🎉 1

elebedeva referenced this issue from a pull request that will close it,

2024-09-13 12:57:38 +00:00

.forgejo: Make 'fumpt' job fail on changed files #1376

fyrchik referenced this issue from a commit

2024-09-13 15:12:32 +00:00

[#1266] .forgejo: Make 'fumpt' job fail on changed files

fyrchik closed this issue

2024-09-13 15:12:33 +00:00