check dirty git tree after running CI jobs #9804

rabi-siddique · 2024-07-30T10:42:10Z

cloudflare-workers-and-pages · 2024-07-30T10:45:48Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`6c0f6cf`
Status:	✅ Deploy successful!
Preview URL:	https://1dc1fdb1.agoric-sdk.pages.dev
Branch Preview URL:	https://rs-check-dirty-tree-post-ci.agoric-sdk.pages.dev

View logs

mhofman

Is there any way we can have some manual tests of this change, maybe as a PR stacked on top of this that creates a dirty file on purpose?

mhofman · 2024-08-01T04:37:10Z

.github/actions/restore-node/action.yml

-        # Fail if `git status` reports anything other than the following:
-        # * an untracked endo-sha.txt from above
-        # * work tree files that have been staged without further changes
-        #   (e.g., package.json or yarn.lock) as indicated by the Y position
-        #   in "XY PATH" being a space


Let's make sure we keep this explanation (with an update) around

mhofman · 2024-08-01T04:41:55Z

.github/actions/restore-node/action.yml

-        fi
+    # Refs: https://github.com/orgs/community/discussions/45342
+    - name: Validate Git Tree in Root Directory
+      if: inputs.path == '.'


I'm confused, why do we need to condition on inputs.path?

The file path can be either . or ./agoric-sdk, which makes it difficult to determine which composite action is being used. Previously, we could specify the path using the working-directory parameter, but this no longer works properly because we now call a separate composite action.

Another option was to concatenate the path value with .github/actions/with-post-step, but it does not work with the uses keyword.

Oh I totally missed the different uses path. I think it's another reason to use an off-the-shelf action for this, no relative path to deal with.

mhofman · 2024-08-01T04:53:03Z

.github/actions/with-post-step/main.cjs

@@ -0,0 +1,22 @@
+// Ref: https://github.com/pyTooling/Actions/blob/main/with-post-step/main.js


Any reason not to use an off the shelf action instead of vendoring something in?

I wasn't sure about which actions to use. After your suggestion, I checked out run-and-post-run-action, action-post-run, and pytooling actions. I ended up choosing pytooling. Thanks for the help :)

mhofman · 2024-08-01T05:01:21Z

.github/actions/restore-node/action.yml

+      if: inputs.path == '.'
+      uses: ./.github/actions/with-post-step
+      with:
+        main: echo "Checking Git tree for changes in the root directory..."


We want to maintain a check after build and before the test runs too. No need to run the test if the tree is dirty to start with.

gibson042 · 2024-08-01T14:52:31Z

.github/actions/restore-node/check-git-status.sh

+# - Files ending with 'junit.xml'
+# - The untracked file 'endo-sha.txt'
+# This setup ensures the Git status only flags unexpected modifications or untracked files outside these specified exceptions.
+changes=$(git status --porcelain | grep -vE 'junit.xml$' | grep -vE '^\?\? endo-sha.txt$')


This should be maximally strict, and if it is using separate grep invocations, it might as well improve readability by avoiding regular expressions where possible:

Suggested change

changes=$(git status --porcelain | grep -vE 'junit.xml$' | grep -vE '^\?\? endo-sha.txt$')

changes=$(git status --porcelain | grep -vE '/junit\.xml$' | grep -vFx '?? endo-sha.txt')

gibson042 · 2024-08-01T14:54:08Z

.github/actions/restore-node/check-git-status.sh

+# Navigate to the specified directory
+cd $1
+# Set verbose execution
+set -x


Let's ensure directory visibility as well.

Suggested change

# Navigate to the specified directory

cd $1

# Set verbose execution

set -x

# Set verbose execution

set -x

# Navigate to the specified directory

cd $1

gibson042 · 2024-08-01T15:01:32Z

.github/actions/restore-node/check-git-status.sh

+# Fail if git status detects changes, ignoring:
+# - Files ending with 'junit.xml'
+# - The untracked file 'endo-sha.txt'


These exceptions should be explained.

Suggested change

# Fail if git status detects changes, ignoring:

# - Files ending with 'junit.xml'

# - The untracked file 'endo-sha.txt'

# Fail if git status detects changes, ignoring:

# - Files ending with 'junit.xml' (cf. scripts/ci-collect-testruns.sh, invoked by ../post-test/action.yml)

# - The untracked file 'endo-sha.txt' (cf. ./action.yml)

@gibson042 Thanks a lot for these handy suggestions. I've made the changes. :)

rabi-siddique · 2024-08-01T17:29:35Z

@mhofman, we have some failing jobs:

https://github.com/Agoric/agoric-sdk/actions/runs/10198519556/job/28213610832?pr=9804
https://github.com/Agoric/agoric-sdk/actions/runs/10198519552/job/28213741871?pr=9804

Should we ignore these files by adding a regex for them?

mhofman · 2024-08-02T18:06:22Z

@mhofman, we have some failing jobs:
Should we ignore these files by adding a regex for them?

I think it means you've uncovered some case where we have some missing entries in .gitignore ?

mhofman

The failing tests likely indicate we're missing some git ignore entries, and I think it should be the likely fix.

I think junit.xml should also be ignore instead of excluded from the check. I think we made an exception for endo-sha.txt, but I don't remember the exact reason. Possibly because it is staged for some of the flows, but I don't know why we couldn't have forced git add in that case. Maybe @michaelfig can shed some lights.

michaelfig

Please use secure bash escaping, and verify that CI passes.

michaelfig · 2024-08-02T19:19:51Z

.github/actions/restore-node/action.yml

+      with:
+        main: |
+          bash ${{ inputs.path }}/.github/actions/restore-node/check-git-status.sh ${{ inputs.path }}
+        post: |
+          bash ${{ inputs.path }}/.github/actions/restore-node/check-git-status.sh ${{ inputs.path }}


Please use defensive shell escaping in case somebody else copies and pastes this code.

Suggested change

with:

main: |

bash ${{ inputs.path }}/.github/actions/restore-node/check-git-status.sh ${{ inputs.path }}

post: |

bash ${{ inputs.path }}/.github/actions/restore-node/check-git-status.sh ${{ inputs.path }}

with:

main: |

bash "$SRC/.github/actions/restore-node/check-git-status.sh" "$SRC"

post: |

bash "$SRC/.github/actions/restore-node/check-git-status.sh" "$SRC"

env:

SRC: ${{ inputs.path }}

michaelfig · 2024-08-02T19:20:49Z

.github/actions/restore-node/check-git-status.sh

+# Set verbose execution
+set -x
+# Navigate to the specified directory
+cd $1


Here, too.

Suggested change

cd $1

cd "$1" || exit $?

Thanks for the suggestions!

michaelfig · 2024-08-02T19:24:29Z

we made an exception for endo-sha.txt, but I don't remember the exact reason.

I know you suggested using git add endo-sha.txt and just ignoring all the indexed files in the restore-node dirty check. I don't remember why that didn't work out.

mhofman · 2024-08-02T19:37:16Z

I know you suggested using git add endo-sha.txt and just ignoring all the indexed files in the restore-node dirty check. I don't remember why that didn't work out.

I'm wondering if we tried simply adding endo-sha.txt to .gitignore?

I know we git stash in some workflows, but I don't think any that relies on endo-sha.txt ?

Even if we needed to support stash, we could do git add -f endo-sha.txt (and any other files needed), followed by git stash --staged

michaelfig · 2024-08-03T03:55:19Z

I'm wondering if we tried simply adding endo-sha.txt to .gitignore?

I'm pretty sure we didn't do that.

I know we git stash in some workflows, but I don't think any that relies on endo-sha.txt ?

I only see it in our after-merge.yml around lerna publish. lerna updates the package.jsons, but I don't see how a git stash would make a difference there. I believe that just adding endo-sha.txt to .gitignore instead would obviate the need to do the stash.

michaelfig

LGTM!

michaelfig · 2024-08-06T03:38:26Z

a3p-integration/.gitignore

@@ -15,6 +15,7 @@ upgrade-test-scripts
 # same for each proposal, an independent project
 proposals/*/.pnp.*
 proposals/*/.yarn/*
+proposals/a:upgrade-next/*


I'm not sure about this one, but if you are, I'm still willing to approve.

I'm still in the process of understanding this, as it appears to be intermittent. Sometimes artifacts are generated when the associated workflow runs, and other times they are not.

…-run

mhofman · 2024-08-06T07:06:02Z

a3p-integration/.gitignore

@@ -15,6 +15,7 @@ upgrade-test-scripts
 # same for each proposal, an independent project
 proposals/*/.pnp.*
 proposals/*/.yarn/*
+proposals/a:upgrade-next/*


That's definitely too broad. Please revert and use a targeted ignore file inside the proposals/a:upgrade-next folder

You should only have to ignore the generated files.

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch from ef0027c to c58c5dc Compare July 30, 2024 10:46

rabi-siddique added the force:integration Force integration tests to run on PR label Jul 30, 2024

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch 10 times, most recently from f51b4a8 to 3411ddc Compare July 30, 2024 13:27

rabi-siddique changed the title ~~ci: check dirty tree in a post run step~~ check dirty git tree after running CI jobs Jul 30, 2024

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch 6 times, most recently from f390344 to 1d7851e Compare July 31, 2024 04:00

rabi-siddique marked this pull request as ready for review July 31, 2024 05:05

rabi-siddique requested a review from mhofman July 31, 2024 05:06

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch from 1d7851e to 712e7d7 Compare July 31, 2024 05:16

mhofman requested changes Aug 1, 2024

View reviewed changes

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch 6 times, most recently from 83f94c5 to 3dacff0 Compare August 1, 2024 11:59

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch 6 times, most recently from d250b2f to 24c19b4 Compare August 1, 2024 13:28

gibson042 reviewed Aug 1, 2024

View reviewed changes

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch from 24c19b4 to e924275 Compare August 1, 2024 15:41

rabi-siddique requested a review from mhofman August 2, 2024 14:29

mhofman approved these changes Aug 2, 2024

View reviewed changes

michaelfig requested changes Aug 2, 2024

View reviewed changes

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch 3 times, most recently from b46c189 to 019fb85 Compare August 5, 2024 04:41

rabi-siddique requested review from michaelfig and mhofman August 5, 2024 06:26

michaelfig approved these changes Aug 6, 2024

View reviewed changes

rabi-siddique added the automerge:rebase Automatically rebase updates, then merge label Aug 6, 2024

rabi-siddique added 2 commits August 6, 2024 04:35

ci: check dirty git tree for jobs

fd78345

ci: use pyTooling/Actions/with-post-step for commands in run and post…

6c0f6cf

…-run

rabi-siddique force-pushed the rs-check-dirty-tree-post-ci-job branch from 019fb85 to 6c0f6cf Compare August 6, 2024 04:35

mergify bot merged commit af5e7ab into master Aug 6, 2024
79 checks passed

mergify bot deleted the rs-check-dirty-tree-post-ci-job branch August 6, 2024 05:31

mhofman reviewed Aug 6, 2024

View reviewed changes

mhofman mentioned this pull request Aug 7, 2024

Move dirty tree check to end of every CI job #5140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check dirty git tree after running CI jobs #9804

check dirty git tree after running CI jobs #9804

rabi-siddique commented Jul 30, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 30, 2024 •

edited

Loading

mhofman left a comment

mhofman Aug 1, 2024

mhofman Aug 1, 2024

rabi-siddique Aug 1, 2024

mhofman Aug 1, 2024

mhofman Aug 1, 2024

rabi-siddique Aug 1, 2024

mhofman Aug 1, 2024

gibson042 Aug 1, 2024

gibson042 Aug 1, 2024

gibson042 Aug 1, 2024

rabi-siddique Aug 1, 2024

rabi-siddique commented Aug 1, 2024

mhofman commented Aug 2, 2024

mhofman left a comment

michaelfig left a comment

michaelfig Aug 2, 2024

michaelfig Aug 2, 2024

rabi-siddique Aug 5, 2024

michaelfig commented Aug 2, 2024

mhofman commented Aug 2, 2024

michaelfig commented Aug 3, 2024

michaelfig left a comment

michaelfig Aug 6, 2024

rabi-siddique Aug 6, 2024

mhofman Aug 6, 2024 •

edited

Loading

		@@ -0,0 +1,22 @@
		// Ref: https://github.com/pyTooling/Actions/blob/main/with-post-step/main.js

	changes=$(git status --porcelain \| grep -vE 'junit.xml$' \| grep -vE '^\?\? endo-sha.txt$')
	changes=$(git status --porcelain \| grep -vE '/junit\.xml$' \| grep -vFx '?? endo-sha.txt')

-      with:
-        main: |
-          bash ${{ inputs.path }}/.github/actions/restore-node/check-git-status.sh ${{ inputs.path }}
-        post: |
-          bash ${{ inputs.path }}/.github/actions/restore-node/check-git-status.sh ${{ inputs.path }}
+      with:
+        main: |
+          bash "$SRC/.github/actions/restore-node/check-git-status.sh" "$SRC"
+        post: |
+          bash "$SRC/.github/actions/restore-node/check-git-status.sh" "$SRC"
+        env:
+          SRC: ${{ inputs.path }}

check dirty git tree after running CI jobs #9804

check dirty git tree after running CI jobs #9804

Conversation

rabi-siddique commented Jul 30, 2024 • edited Loading

cloudflare-workers-and-pages bot commented Jul 30, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rabi-siddique commented Aug 1, 2024

mhofman commented Aug 2, 2024

mhofman left a comment

Choose a reason for hiding this comment

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfig commented Aug 2, 2024

mhofman commented Aug 2, 2024

michaelfig commented Aug 3, 2024

michaelfig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

rabi-siddique commented Jul 30, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 30, 2024 •

edited

Loading

mhofman Aug 6, 2024 •

edited

Loading