Andrew7234/workflow nits #657

Andrew7234 · 2024-03-08T01:15:34Z

do not merge (yet)

This PR has misc tweaks/fixes for things that were broken in my local dev setup (zsh on Mac OS). I'm not sure if all of it should be merged; just wanted to start a discussion. I tried to separate different issues by commit

Andrew7234 · 2024-03-08T01:17:50Z

tests/e2e_regression/ensure_consistent_config.sh

-for range in $(find "$directory" -type d); do
+for range in $(find "tests/e2e_regression" -maxdepth 1 -mindepth 1 -type d); do


./tests/e2e_regression/ensure_consistent_config.sh: line 26: directory: unbound variable

woa how did this used to work

🤨 yeah the old code makes no sense

changed in #665, this script now takes the name of a suite as a parameter instead of searching around

pro-wh · 2024-03-08T01:18:42Z

tests/e2e_regression/run.sh

@@ -89,7 +89,7 @@ for ((i = 0; i < nCases; i++)); do
      >/tmp/pretty 2>/dev/null &&
      cp /tmp/pretty "$outDir/$name.body" || true
    # Sanitize the current timestamp out of the response header so that diffs are stable
-    sed -i -E 's/^(Date|Content-Length|Last-Modified): .*/\1: UNINTERESTING/g' "$outDir/$name.headers"
+    sed -i '' -E 's/^(Date|Content-Length|Last-Modified): .*/\1: UNINTERESTING/g' "$outDir/$name.headers"


shouldn't be related to zsh, maybe mac's coreutils. this script has a shebang set for bash anyway

uhh I remember we've had back-and-forth with sed -i '' before :(
This doesn't work on GNU sed (which is what I have locally, and what CI has). It interprets the '' as the sed script to run. You can try it yourself: docker run -it bash, then echo foo >/tmp/x; sed -i '' 's/o/a/' /tmp/x.

You could try instead:

using echo foo >/tmp/x; sed --in-place='' 's/o/a/' /tmp/x (though I think that's not supported on MacOS?)

creating a little helper function that reimplements sed -i without using -i (*), or using -i based on some OS-specific check

leaving this as-is and using GNU sed on your machine?

(*) something like

sed_i() { local file="${@: -1}" # Get the last argument (file path) local script="${@: -2:1}" # Get the penultimate argument (sed script) local flags="${@:1:$#-2}" # Get flags (all arguments except the last two) local temp_file="$(mktemp)" sed "${flags[@]}" "$script" "$file" > "$temp_file" mv "$temp_file" "$file" # Overwrites the original file }

@Andrew7234 I think you said you're using GNU sed on your machine currently? So that resolves this thread for now (remember to undo the change), and you can submit the rest of the PR at least. It's useful stuff, would be a shame if it rotted away.

Andrew7234 · 2024-03-08T01:19:03Z

Makefile

-	# output every time the spec changes. \
+	# The result of the "spec" test is a special case. It should always match the
+	# current openAPI spec file, so we symlink it to avoid having to update the expected
+	# output every time the spec changes.


I think the \ at the end of commented lines caused subsequent non-comment lines to be treated as a comment and skipped

what was the intention with these? run the whole thing in one shell instead of several?

hm I'm not sure.. maybe @mitjat knows? I'm also curious as to whether it works locally for y'all as-is

just tried on linux with GNU bash, version 5.2.21(1)-release and GNU Make 4.3, the lower lines seem to run normally

Interesting. I don't remember why the \ are there, unfortunately. If this works locally and in CI, LGTM.

Although I'd say:

If we're doing this, let's remove all \ in this recipe so we're not using a weird mix.

Prepend every # with @ or else make will echo those comments to the console every time we run the recipe.

this is moved to a new script e2e_regression/accept.sh in #665, which makes it no longer necessary to use \s. it also should quiet down the logging of every line

mitjat

Nice, thank you for all of these! Approving because LGTM (pending comments), but happy to look at it again if you wanted to add more to the same PR.

mitjat · 2024-03-08T02:03:24Z

tests/e2e_regression/ensure_consistent_config.sh

-for range in $(find "$directory" -type d); do
+for range in $(find "tests/e2e_regression" -maxdepth 1 -mindepth 1 -type d); do


🤨 yeah the old code makes no sense

mitjat · 2024-03-08T02:03:28Z

Makefile

-	# output every time the spec changes. \
+	# The result of the "spec" test is a special case. It should always match the
+	# current openAPI spec file, so we symlink it to avoid having to update the expected
+	# output every time the spec changes.


Interesting. I don't remember why the \ are there, unfortunately. If this works locally and in CI, LGTM.

Although I'd say:

If we're doing this, let's remove all \ in this recipe so we're not using a weird mix.

Prepend every # with @ or else make will echo those comments to the console every time we run the recipe.

mitjat · 2024-03-08T02:23:19Z

tests/e2e_regression/run.sh

@@ -89,7 +89,7 @@ for ((i = 0; i < nCases; i++)); do
      >/tmp/pretty 2>/dev/null &&
      cp /tmp/pretty "$outDir/$name.body" || true
    # Sanitize the current timestamp out of the response header so that diffs are stable
-    sed -i -E 's/^(Date|Content-Length|Last-Modified): .*/\1: UNINTERESTING/g' "$outDir/$name.headers"
+    sed -i '' -E 's/^(Date|Content-Length|Last-Modified): .*/\1: UNINTERESTING/g' "$outDir/$name.headers"


uhh I remember we've had back-and-forth with sed -i '' before :(
This doesn't work on GNU sed (which is what I have locally, and what CI has). It interprets the '' as the sed script to run. You can try it yourself: docker run -it bash, then echo foo >/tmp/x; sed -i '' 's/o/a/' /tmp/x.

You could try instead:

using echo foo >/tmp/x; sed --in-place='' 's/o/a/' /tmp/x (though I think that's not supported on MacOS?)

creating a little helper function that reimplements sed -i without using -i (*), or using -i based on some OS-specific check

leaving this as-is and using GNU sed on your machine?

(*) something like

sed_i() { local file="${@: -1}" # Get the last argument (file path) local script="${@: -2:1}" # Get the penultimate argument (sed script) local flags="${@:1:$#-2}" # Get flags (all arguments except the last two) local temp_file="$(mktemp)" sed "${flags[@]}" "$script" "$file" > "$temp_file" mv "$temp_file" "$file" # Overwrites the original file }

remove old sourcify cache makefile script fix(?) fix script [do not merge] zsh-compatible version of run.sh evmverifier: remove extraneous fields add changelog

Andrew7234 requested review from mitjat, pro-wh and ptrus as code owners March 8, 2024 01:15

Andrew7234 commented Mar 8, 2024

View reviewed changes

pro-wh reviewed Mar 8, 2024

View reviewed changes

Andrew7234 commented Mar 8, 2024

View reviewed changes

mitjat approved these changes Mar 8, 2024

View reviewed changes

Andrew7234 force-pushed the andrew7234/workflow-nits branch from f05a4f3 to beedf21 Compare March 26, 2024 02:56

workflow nits

2c4f68f

remove old sourcify cache makefile script fix(?) fix script [do not merge] zsh-compatible version of run.sh evmverifier: remove extraneous fields add changelog

Andrew7234 force-pushed the andrew7234/workflow-nits branch from beedf21 to 2c4f68f Compare March 27, 2024 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Andrew7234/workflow nits #657

Andrew7234/workflow nits #657

Andrew7234 commented Mar 8, 2024 •

edited

Loading

Andrew7234 Mar 8, 2024

pro-wh Mar 8, 2024

mitjat Mar 8, 2024

pro-wh Mar 27, 2024

pro-wh Mar 8, 2024

mitjat Mar 8, 2024

mitjat Mar 23, 2024

Andrew7234 Mar 8, 2024

pro-wh Mar 8, 2024

Andrew7234 Mar 8, 2024

pro-wh Mar 8, 2024

mitjat Mar 8, 2024

pro-wh Mar 27, 2024 •

edited

Loading

mitjat left a comment

mitjat Mar 8, 2024

mitjat Mar 8, 2024

mitjat Mar 8, 2024

		for range in $(find "$directory" -type d); do
		for range in $(find "tests/e2e_regression" -maxdepth 1 -mindepth 1 -type d); do

Andrew7234/workflow nits #657

Are you sure you want to change the base?

Andrew7234/workflow nits #657

Conversation

Andrew7234 commented Mar 8, 2024 • edited Loading

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

pro-wh Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

mitjat 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

Andrew7234 commented Mar 8, 2024 •

edited

Loading

pro-wh Mar 27, 2024 •

edited

Loading