search

privacy-scaling-explorations / snark-artifacts

A streamlined mechanism for distributing SNARK artifacts.
https://snark-artifacts.pse.dev
MIT License
4 stars 2 forks source link

ci: fix update update-cloudfront.sh #117

Closed sripwoud closed 4 months ago

sripwoud commented 4 months ago

I missed this when reviewing the PR: if we store the result of ls packages/ | grep -v 'artifacts\|cli' before looping, the shell we split it line by line before looping, which breaks the loop. I suggest sticking to command subsitution, as in this case newlines are deleted and the for loop will work as intended

Bash performs the expansion by executing command in a subshell environment and replacing the command substitution with the standard output of the command, with any trailing newlines deleted. Embedded newlines are not deleted, but they may be removed during word splitting

image

changeset-bot[bot] commented 4 months ago

⚠️ No Changeset found

Latest commit: 420ff273c94cb5d3ec41d7e3b2489a4167e979a9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

ntampakas commented 4 months ago

Hey @sripwoud , I can vote up for the suggested change, but to be honest i cannot see the bug, or are you talking about a different scenario where folders have spaces or special characters? Feeding the result directly in the loop, will also break it, if that' s the case (while - read should be used instead in that scenario).

I also tried to reproduce the result of the commands in your screenshot, but the output is the same: image

In addition, the current implemented code seems to work fine, otherwise wouldn' t be an issue during the workflow execution? image

Please let me know if i missed or misinterpreted anything in your PR :)

sripwoud commented 4 months ago

i cannot see the bug

hmm what is your shell? Maybe I have some shell configuration I forgot about?

otherwise wouldn' t be an issue during the workflow execution?

indeed, I was not sure if I should really suggest a fix for this.

Let's close this, if the script works in the ci env no need to change anything

ntampakas commented 4 months ago

Tested on Ubuntu 24.04 LTS with GNU bash, version 5.2.21(1)-release (x86_64-pc-linux-gnu).