Skip to content

Conversation

@vinckama
Copy link
Contributor

@vinckama vinckama commented Nov 13, 2024

Add gofmt and go mod tidy in the CI to make sure go files are always formatted. Before fixing formatting, the CI failed, it demonstrates that the formatter works as expected:

Screenshot 2024-11-13 at 15 08 56

@vinckama vinckama marked this pull request as ready for review November 13, 2024 12:56
@vinckama vinckama requested a review from a team as a code owner November 13, 2024 12:56
@vinckama vinckama changed the title Add gofmt in CI Add gofmt & go mod tidy in CI Nov 13, 2024
Copy link
Contributor

@fbryden fbryden Nov 13, 2024

Choose a reason for hiding this comment

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

I'm not exactly sure how this works in a CI...but -w will "fix" the code. Is -d the more "suitable" option here, which displays the issues rather than fixes them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the challenge here is that the failed command is github do not make the test to fail. See this discussion, I think it is still not implemented yet

Copy link
Contributor Author

@vinckama vinckama Nov 13, 2024

Choose a reason for hiding this comment

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

I tried on 1e49fd0, to double check the behavior, moving the PR to draft again

edit: after testing gofmt -s -d ., the CI does not fail in that case:
Screenshot 2024-11-13 at 15 03 29

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I was going to suggest something I thought was smart - but I just found it in the thread you linked lol

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm currently testing this in my own PR:

  script:
    # Only the sds-go folder needs to be checked since generation-checks takes care of the generated code
    - output=$(gofmt -d -s sds-go)
    - if [[ -n "$output" ]]; then echo "gofmt failed"; echo "$output"; exit 1; fi

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script suggested is more complicated than the current one. I'm not sure it's a real improvement

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I'd missed your suggestion - I agree with you and I'll be following that in my own PR.

@vinckama vinckama marked this pull request as draft November 13, 2024 14:01
@vinckama vinckama requested a review from fbryden November 13, 2024 14:09
@vinckama vinckama marked this pull request as ready for review November 13, 2024 14:09
@vinckama
Copy link
Contributor Author

/merge

@dd-devflow
Copy link

dd-devflow bot commented Nov 13, 2024

Devflow running: /merge

View all feedbacks in Devflow UI.


2024-11-13 16:11:37 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2024-11-13 16:11:41 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.


2024-11-13 16:19:59 UTC ℹ️ MergeQueue: pull request added to the queue

The median merge time in main is 0s.


2024-11-13 17:42:34 UTCMergeQueue: This merge request was updated

This PR is rejected because it was updated

Copy link
Contributor

@fbryden fbryden left a comment

Choose a reason for hiding this comment

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

LGTM

@vinckama vinckama enabled auto-merge (squash) November 13, 2024 17:42
@vinckama vinckama merged commit c296c6a into main Nov 13, 2024
3 checks passed
@vinckama vinckama deleted the vincent.roy/2306 branch November 13, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants