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

ci: manual job for updating in-repo go mods; ci to warn if files changed #4667

Merged
merged 50 commits into from
Aug 28, 2023

Conversation

pysel
Copy link
Member

@pysel pysel commented Mar 19, 2023

Closes: #4660

Description

This PR introduces a manual CI job to update go.mod for osmomath, osmoutils, x/ibc-hooks and x/epochs if changed.

The changes introduced in this PR include:

  • Manual CI that runs a bash script for updating go mods
  • CI that comments in PRs with instructions for running if files are updated

Tested manually on a fork

@github-actions github-actions bot added the T:CI label Mar 19, 2023
@pysel pysel added V:state/compatible/no_backport State machine compatible PR, depends on prior breaks and removed T:CI labels Mar 19, 2023
@github-actions github-actions bot added the T:CI label Mar 19, 2023
@pysel pysel force-pushed the ruslan/go-mod-bug branch from ec81947 to d465e07 Compare March 19, 2023 07:04
@pysel pysel force-pushed the ruslan/go-mod-bug branch from 98abbdc to a6a3d85 Compare March 19, 2023 13:58
@pysel
Copy link
Member Author

pysel commented Mar 19, 2023

hey @p0mvn. A quick update on this:

There is a problem with this workflow when a push with changes is a force push. Sometimes, if a force push removes the latest commit in PR, and sets the newly pushed commit as the latest one, this github workflow will fail, because github.event.before (which is used to find the commit before the push), will point to the commit that was deleted when force-pushing

We could solve that by giving the ability to the developer to run this workflow manually, however, I think this completely ruins the point of this automation.

Another idea is to run the updating script ALWAYS when a force push happens, updating all 4 go.mod folders, but I am also not a fan of this idea.

The third idea I have, is to change the commit before in this workflow to be the latest commit on main branch. This will remove the need to figure out ways to deal with force pushes, and it seems to me that it will also be logically fine.

wdyt?

@@ -33,14 +33,29 @@ jobs:
run: |
echo "CURRENT_BRANCH=$(echo ${GITHUB_REF} | sed 's|refs/heads/||')" >> $GITHUB_OUTPUT

- name: Run script
# This step fixes a bug, due to which the push to any branch, for example: to person-A/name, triggered this workflow on branch person-b/name,
Copy link
Member

Choose a reason for hiding this comment

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

nit: can we put this before "Setup Go"

Comment on lines 36 to 51
# This step fixes a bug, due to which the push to any branch, for example: to person-A/name, triggered this workflow on branch person-b/name,
# and updated the go.mod on branch person-B/name with commit from branch person-A/name
- name: Check if the current branch contains commit that triggered the workflow
id: valid_branch
run: |
CURRENT_BRANCH=${{ steps.extract_branch.outputs.CURRENT_BRANCH }}
COMMIT_EXISTS_ON=$(echo $(git branch --contains ${{ github.event.after}} ) | grep $CURRENT_BRANCH)

if [[ "$COMMIT_EXISTS_ON" == *"$CURRENT_BRANCH"* ]];
then
echo "VALID=1" >> $GITHUB_OUTPUT
fi

- name: Run script
if: ${{ steps.valid_branch.outputs.VALID }} == 1
Copy link
Member

Choose a reason for hiding this comment

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

So assume I'm on branch roman/A and you are on ruslan/B

Then, I modify osmoutils on my branch. Then, this workflow will run for both roman/A and ruslan/B. However, it will not continue past this step on ruslan/B. Is my understanding correct?

I'm wondering if there is a way to not run this job on other branches at all to avoid having a queue in CI.

Let me know what you think

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is right. However, as mentioned here, I haven't seen such behavior by changing, for example, osmoutils on my local branch. I checked the Action tab, which showed me, that this workflow was running only for my branch.

A more detailed explanation you may find in the attached link above

Copy link
Member Author

@pysel pysel Mar 20, 2023

Choose a reason for hiding this comment

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

let me know what you think on removing main from this workflow's triggers

@p0mvn
Copy link
Member

p0mvn commented Mar 19, 2023

hey @p0mvn. A quick update on this:

There is a problem with this workflow when a push with changes is a force push. Sometimes, if a force push removes the latest commit in PR, and sets the newly pushed commit as the latest one, this github workflow will fail, because github.event.before (which is used to find the commit before the push), will point to the commit that was deleted when force-pushing

We could solve that by giving the ability to the developer to run this workflow manually, however, I think this completely ruins the point of this automation.

Another idea is to run the updating script ALWAYS when a force push happens, updating all 4 go.mod folders, but I am also not a fan of this idea.

The third idea I have, is to change the commit before in this workflow to be the latest commit on main branch. This will remove the need to figure out ways to deal with force pushes, and it seems to me that it will also be logically fine.

wdyt?

Thanks for the breakdown of the options.

We could solve that by giving the ability to the developer to run this workflow manually

This makes sense to me as a temporary solution

Another idea is to run the updating script ALWAYS when a force push happens, updating all 4 go.mod folders, but I am also not a fan of this idea.

What do you mean by all go.mod folders? Do you mean that If I make a change to osmoutils, it will update go.mod for all of osmoutils, osmomath etc?

This seems fine to me. It is actually preferable that you are always depending on the modules directly in your branch.

The third idea I have, is to change the commit before in this workflow to be the latest commit on main branch. This will remove the need to figure out ways to deal with force pushes, and it seems to me that it will also be logically fine.

This is not what we would like to have. In this case, I can update osmoutils and have e2e CI fail because I'm not depending on the osmoutils change from my PR

@pysel
Copy link
Member Author

pysel commented Mar 20, 2023

What do you mean by all go.mod folders? Do you mean that If I make a change to osmoutils, it will update go.mod for all of osmoutils, osmomath etc?

Yes, but only when a force push happens, to avoid checking git diff with the latest commit sha, which could have been deleted by force pushing

This is not what we would like to have. In this case, I can update osmoutils and have e2e CI fail because I'm not depending on the osmoutils change from my PR

Hmm, I do not really understand this. Commit After will still be the latest commit on your local branch, which will be used to go get the required dependencies. By changing commit before to latest on main, we will basically check git diff between your latest branch state and the latest main state.

In comparison, right now we are checking git diff between the latest commit - 1 and latest commit, where latest commit - 1 is commit before and latest commit is commit after

This is what I mean by commit before and commit after, by the way

CC: @p0mvn

@pysel pysel force-pushed the ruslan/go-mod-bug branch 3 times, most recently from d1ffc17 to 86c200f Compare March 20, 2023 07:08
@pysel pysel marked this pull request as ready for review March 21, 2023 04:00
@pysel pysel marked this pull request as draft March 21, 2023 04:03
@pysel pysel force-pushed the ruslan/go-mod-bug branch 9 times, most recently from 7b91156 to cf3528b Compare March 21, 2023 08:30
@pysel pysel changed the title bug: fix go mod workflow bug bug: fix go mod workflow bugs Mar 21, 2023
@pysel pysel mentioned this pull request Mar 22, 2023
@pysel pysel force-pushed the ruslan/go-mod-bug branch from 8c5f41a to 82b2c80 Compare March 30, 2023 03:09
@pysel pysel force-pushed the ruslan/go-mod-bug branch from e38f22d to 63cf2ab Compare July 6, 2023 11:05
@github-actions
Copy link
Contributor

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!

@github-actions github-actions bot added the Stale label Jul 21, 2023
@pysel pysel removed the Stale label Jul 22, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 6, 2023

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!

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@p0mvn p0mvn left a comment

Choose a reason for hiding this comment

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

@pysel great work on this.

I made some changes to make this manual instead of automatic:
e3dbdaa

Also added a job that would comment where to manually run the CI whenever one of the modules updated.

Did manual test here: p0mvn#4

Let's merge and test this out

@p0mvn p0mvn changed the title auto: go.mod auto update workflow when changing local dependencies ci: manual job for updating in-repo go mods; ci to warn if files changed Aug 28, 2023
@p0mvn p0mvn merged commit 08e9d0a into main Aug 28, 2023
@p0mvn p0mvn deleted the ruslan/go-mod-bug branch August 28, 2023 10:07
nicolaslara pushed a commit that referenced this pull request Aug 31, 2023
…ged (#4667)

* add suggested change

* check for valid branch

* change-wip

* debugging

* wip

* debugging

* debugging

* debugging

* debugging

* debugging

* debugging

* debugging

* remove change in osmoutils

* Update .github/workflows/go-mod-auto-bump.yml

* Update .github/workflows/go-mod-auto-bump.yml

* change

* remove debug echos

* docs: correctly name a step

* change

* change osmoutils

* add token

* git diff with latest main commit

* remove echo

* remove osmoutils changes

* improve docs

* remove main branch from triggers

* fix bug: running on wrong branch

* remove legacy conditions

* PAT

* v3 checkout

* osmoutils change

* bring back script

* change

* config

* change

* better commit message + try w/o token

* change

* bring back token

* change

* remove osmoutils change

* only commit+push changes when they were added

* change

* nit

* reset change

* tidy and changelog

* restore go.modsum

* better CHANGELOG

* use mod/sum from main

* make manual

* Update CHANGELOG.md

---------

Co-authored-by: Jacob Gadikian <[email protected]>
Co-authored-by: Roman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:no-changelog T:build T:CI V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: go mod auto-update to commit to custom branch when dependency module is updated
6 participants