checkout icon indicating copy to clipboard operation
checkout copied to clipboard

wrap pipeline commands for submoduleForeach in quotes

Open jokreliable opened this issue 3 years ago • 1 comments

Update the commands passed to submoduleForeach to make sure the expected behavior is happening. Without the quotes, only the first part of those pipelines are handled by git-submodule foreach, and the rest of the pipeline is handled outside of the loop, which is likely not the intended behavior.

jokreliable avatar Oct 14 '22 21:10 jokreliable

This should address #354 and a few other issues related to submodule(s).

jokreliable avatar Oct 14 '22 21:10 jokreliable

@cory-miller?

jsoref avatar Nov 09 '22 19:11 jsoref

I sent a PR to fix an unrelated CI issue. In the meantime, could you please update the JS bundle with npm run build?

cory-miller avatar Nov 10 '22 20:11 cory-miller

Updated! (and thank you for the pointers -- never used typescript before...)

jokreliable avatar Nov 11 '22 00:11 jokreliable

yikes - that's a lot of failures.. not sure I can help much with making the CI pass... They all seem to point to some sort of dependency resolver for npm (and i'm guessing it's part of the package-lock.json that i included as part of my commit after i updated the dist file... any pointer/docs I can go look at to get educated on this?

jokreliable avatar Nov 11 '22 02:11 jokreliable

Sorry for the delay and bad info. I assumed this Action worked similarly to others owned by my team, but that doesn't seem to be the case. Instead of npm install, you can use npm ci. These commands should fix the deps problems in CI.

git checkout origin/main -- package-lock.json && \
  npm ci && npm run format && npm run build

I did the same in https://github.com/actions/checkout/pull/1010 and the CI now fails due to submodule command problems: https://github.com/actions/checkout/actions/runs/3463800067/jobs/5784532604

The failing CI is https://github.com/actions/checkout/blob/5c3ccc22eb2c950a0fa5bc7c47190d8e3f7e681a/.github/workflows/test.yml#L96-L102

The exact error copied below in case you can't see.

  /usr/bin/git submodule foreach --recursive "git config --local --name-only --get-regexp 'url\.https\:\/\/github\.com\/\.insteadOf' && git config --local --unset-all 'url.https://github.com/.insteadOf' || :"
  Entering 'submodule-level-1'
  path='submodule-level-1'; "git config --local --name-only --get-regexp 'url\.https\:\/\/github\.com\/\.insteadOf' && git config --local --unset-all 'url.https://github.com/.insteadOf' || :": 1: git config --local --name-only --get-regexp 'url\.https\:\/\/github\.com\/\.insteadOf' && git config --local --unset-all 'url.https://github.com/.insteadOf' || :: not found

Not having worked in actions/checkout before, I am still trying to figure out why this is broken. Let me know if you have insight. I'm also reaching out to other on my team that have worked in this action before.

cory-miller avatar Nov 14 '22 18:11 cory-miller

I expect that git, instead of just running the code in there as a shell command (the part i wrapped quotes around), it is trying to use the whole thing (including spaces and all that) as $0 in exec... so not quite as simple as that. There might need to be some trickery required, or some extensive rework of the code to make it work... I'll have a look and will try a couple of different things to see what works...

jokreliable avatar Nov 15 '22 23:11 jokreliable

@jokreliable: I'd suggest you try creating a branch in your repository named releases/test/submoduleForeach-pipelinefixes and push to it.

The workflows avoid testing on things that aren't main or releases/*, so if you want to improve your iteration speed, using one of those two would help. Then you wouldn't have to wait for someone here to approve the pushes to this branch.

jsoref avatar Nov 15 '22 23:11 jsoref

done! (I had to sync specific branches into my repo, but the CI cleared all green across the board... (see https://github.com/jokreliable/checkout/actions/runs/3475087417 for output) -- will update this branch to match...

jokreliable avatar Nov 16 '22 00:11 jokreliable

well, outside of the comment present in this branch, it matches... (i think)

jokreliable avatar Nov 16 '22 00:11 jokreliable