CI: Catches merge conflicts in go.sum files

[anhductn2001]

Closes: #6538

What is the purpose of the change

This Job will run go mod tidy and compare with current code. If there are any unresolved changes (eg merge conflict) it will notify us

Testing and Verifying

Have tested on fork repo here.

[anhductn2001]

Hey @pysel, we just need to check "merge conflict not resolved" in go.sum is enough. We will not need to find it in all files as you mentioned in #6538 ... because if these special characters appear, it will cause an error in go test jobs like I have test here

[mattverse]

I didn't quite understand how this is suppose to "notify" us, was your intention to be creating a new PR with the diffs? Or is it suppose to show us the diffs?

If so, instead of showing diffs, I feel like we should be directly running go mod tidy within workflow so that we don't have to fix the conflict!

[pysel]

I didn't quite understand how this is suppose to "notify" us, was your intention to be creating a new PR with the diffs? Or is it suppose to show us the diffs?

I am pretty sure that the way this notifies us is by failing build workflow if go mod is not tidied, since if there is any diff after running go mod tidy, the build workflow exits with code 1

[pysel]

thanks a lot for PR! I have a few concerns:

1. tidy step is happening after the build of osmosisd finishes. Incorrectly formatted go.sum files can fail the build process on the earlier stage of workflow's execution. Hence, if there are conflicts in go.sum files, this workflow can actually fail prior to tidy step, which won't make it obvious what the problem is. So if we go with this approach, I suggest moving the tidy step before building steps
2. This approach would only find conflicts in the go.sum file of the root of the repository. I suggest running makefile target recently introduced, namely tidy-workspace, in order to run go mod tidy in every submodule
3. Sometimes go mod is not formatted, but it does not mean that a repository is in a bad state. In these cases, it would be inconvenient that build stage in CI fails only because there is slightly not up-to-date go mod in the repository.

To sum up, I still think that if there is a linter that can catch these errors and if so, just enable it. If not, and we are using go mod tidy approach, I do not think it is good to fail CI if it is not a definite go.sum conflict we are facing.

Maybe there are other options to solving this problem that can mitigate it. One approach is to run tidy-workspace and push changes to a PR's branch every time, say, a PR is opened for review. Even better, if PR got one approval (though, it would in a sense make that approval stale)

[anhductn2001]

Thank you for your concern @pysel! I would move the tidy before build step and use tidy-workspace instead go mod tidy

[anhductn2001]

Currently I haven't found any linter that allows us to check this

[anhductn2001]

Now tidy will run make tidy-workspace then create a commit directly to the branch. What do you think @pysel ?

[pysel]

hey @anhductn2001! thanks for addressing review. Is it possible not to run tidy step when a PR is merged? It would mean that every PR review essentially reviews a slightly different version of what is going to be committed to main, even though it only tidies the workspace

[pysel]

will this fail the workflow if make tidy-workspace introduce no changes?

[anhductn2001]

It will not fail @pysel

[anhductn2001]

Can you explain more clearly the meaning of Is it possible not to run tidy step when a PR is merged?

[pysel]

this workflow runs on push to main (in other words, when a PR is merged). So I am worried that after some PR gets merged, this workflow will commit changes introduced by tidy-workspace directly to main (if I understand correctly), if any

[anhductn2001]

Ok, I understand. My opinion is:

1. Maybe we will never push a commit directly to the main branch, right? So the case of pushing directly to the main can be eliminated.
2. In case of merging PR to main, CI will be run in PR first so it will create a commit on the PR branch so I see no problem here.

[anhductn2001]

hey @anhductn2001! thanks for addressing review. Is it possible not to run tidy step when a PR is merged? It would mean that every PR review essentially reviews a slightly different version of what is going to be committed to main, even though it only tidies the workspace

Yes, I could create a separate workflow for tidy and have it execute only on PR.

[github-actions]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

[mattverse]

ACK, do we have a run of failed CI @anhductn2001 ?

[anhductn2001]

What CI failed? @mattverse

[anhductn2001]

If you don't want the log to appear every time you run, I can clear it after each run.
I know the log option in our e2e, but in testing e2e we do not use log to check the status and init-chain, but for dockernet we only have a way to save it to the local machine to do this.

[mattverse]

@anhductn2001 Oh I meant do we have a failed run of this CI just to make sure that this CI works as intended!

[github-actions]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!