Create configlet-sync.yml #212

jagdish-15 · 2025-06-24T05:09:51Z

Pull Request

See forum topic here

Add configlet-sync.yml

.github/workflows/configlet-sync.yml

IsaacG · 2025-06-24T05:36:11Z

.github/workflows/configlet-sync.yml

+          configlet_raw_output="$(./bin/configlet sync --tests | tee .github/sync-test-output.txt)"
+
+          echo "configlet output:"
+          echo "$configlet_raw_output"
+
+          echo '```' > .github/sync-test-output.txt
+          echo "$configlet_raw_output" >> .github/sync-test-output.txt
+          echo '```' >> .github/sync-test-output.txt
+
+          echo "output<<EOF" >> "$GITHUB_OUTPUT"
+          echo "$configlet_raw_output" >> "$GITHUB_OUTPUT"
+          echo "EOF" >> "$GITHUB_OUTPUT"


Is there a reason the .github directory would be used as a scratch location? That path should be used for github related configurations. Usually /tmp or a tempfile is used for this sort of thing.

Additionally, you write the contents to this file (L55) then immediately overwrite the contents a few lines later (L60).

Suggested change

configlet_raw_output="$(./bin/configlet sync --tests | tee .github/sync-test-output.txt)"

echo "configlet output:"

echo "$configlet_raw_output"

echo '```' > .github/sync-test-output.txt

echo "$configlet_raw_output" >> .github/sync-test-output.txt

echo '```' >> .github/sync-test-output.txt

echo "output<<EOF" >> "$GITHUB_OUTPUT"

echo "$configlet_raw_output" >> "$GITHUB_OUTPUT"

echo "EOF" >> "$GITHUB_OUTPUT"

configlet_raw_output="$(./bin/configlet sync --tests)"

printf "configlet output:\n%s\n" "$configlet_raw_output"

printf '```\n%s\n```\n' "$configlet_raw_output" > .github/sync-test-output.txt

printf 'output<<EOF\n%s\nEOF\n' "$configlet_raw_output" > "$GITHUB_OUTPUT"

Is the output needed in two files?

Additionally, you write the contents to this file (L55) then immediately overwrite the contents a few lines later (L60).

You're absolutely right, I had meant to update line 55 so it only saved the variable, but I missed it. Thanks for catching that!

Also, really appreciate the other suggestions. They’ve definitely helped clean up and shorten the code. I agree that using a temp folder makes more sense here, so I’ll go ahead and make that change as well.

/tmp and the mktemp tool, not a local directory named "temp"

Do you need the output in two places?

Does output > $GITHUB_OUTPUT make sense or should you be populating the contents of that variable?

Do you need to write the output to a file at all or can you use $GITHUB_OUTPUT to share content between steps? It seems like the whole purpose of $GITHUB_OUTPUT is to avoid needing temp files.

IsaacG · 2025-06-24T06:01:50Z

.github/workflows/configlet-sync.yml

+          echo "$configlet_raw_output" >> .github/sync-test-output.txt
+          echo '```' >> .github/sync-test-output.txt
+
+          echo "output<<EOF" >> "$GITHUB_OUTPUT"


If I'm understanding correctly, this variable is supposed to contain an output string, and is not meant to be used as a filename.

Exactly, the configlet_raw_output variable was meant to capture the output of the configlet command, and now it works as intended after you caught the issue at line 55!

I'm referring to the $GITHUB_OUTPUT variable.

.github/workflows/configlet-sync.yml

Co-authored-by: Isaac Good <[email protected]>

IsaacG · 2025-06-24T15:10:57Z

.github/workflows/configlet-sync.yml

+          configlet_raw_output="$(./bin/configlet sync --tests)"
+
+          printf "configlet output:\n%s\n" "$configlet_raw_output"
+          mkdir -p temp


/tmp or the mktemp tool, not a local temp file ;)

ErikSchierboom

Lovely idea! I've left some comments

ErikSchierboom · 2025-06-24T18:53:29Z

.github/workflows/configlet-sync.yml

+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4


Please pin actions to SHA's, not tags. See https://exercism.org/docs/building/github/gha-best-practices#h-pin-actions-to-shas

ErikSchierboom · 2025-06-24T18:53:52Z

.github/workflows/configlet-sync.yml

+        run: ./bin/configlet sync --docs --metadata --filepaths -u -y
+
+      - name: Create pull request if changes
+        uses: peter-evans/create-pull-request@v5


Please also pin this to a SHA

ErikSchierboom · 2025-06-24T18:54:24Z

.github/workflows/configlet-sync.yml

+
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4


Please pin to a SHA

ErikSchierboom · 2025-06-24T18:58:53Z

.github/workflows/configlet-sync.yml

+
+      - name: Format test sync output and find existing issue
+        id: find_issue
+        if: ${{ !contains(steps.sync_test.outputs.output, 'Every exercise has up-to-date tests!') }}


Instead of relying on the output, I would suggest using the exit code of ./bin/configlet sync --tests to determine if anything is unsynced. I've just checked and it looks like it is non-zero when there are out-of-date tests, and zero when there aren't. You might want to double check, but I would recommend using that if true.

ErikSchierboom · 2025-06-24T18:59:43Z

.github/workflows/configlet-sync.yml

+
+      - name: Create or Update issue if tests are not synced
+        if: ${{ !contains(steps.sync_test.outputs.output, 'Every exercise has up-to-date tests!') }}
+        uses: peter-evans/create-issue-from-file@v5


Please pin this to as SHA

.github/workflows/configlet-sync.yml

kahgoh · 2025-06-25T13:18:08Z

.github/workflows/configlet-sync.yml

+on:
+  workflow_dispatch:
+  schedule:
+    - cron: '0 0 15 * *'
+


I'd suggest removing the schedule and dispatch and allowing the tracks to "opt-in" by setting their own schedules at the track's workflow level.

Co-authored-by: Isaac Good <[email protected]>

jagdish-15 · 2025-06-26T19:43:43Z

@IsaacG @ErikSchierboom @kahgoh

I've incorporated all the changes suggested by you! Please let me know if there's anything else that needs adjustment.

One thing to note: I had to use set +e and set -e to prevent the workflow from exiting early when configlet returns a non-zero exit code. You might want to take a quick look at that part.

.github/workflows/configlet-sync.yml

Co-authored-by: Glenn Jackman <[email protected]>

IsaacG · 2025-06-27T01:07:20Z

.github/workflows/configlet-sync.yml

+        id: sync_test
+        shell: bash {0}
+        run: |
+          configlet_raw_output="$(./bin/configlet sync --tests 2>&1)"


Is there useful output in STDERR?

IsaacG · 2025-06-27T01:07:48Z

.github/workflows/configlet-sync.yml

+        run: |
+          configlet_raw_output="$(./bin/configlet sync --tests 2>&1)"
+          exit_code=$?
+          echo "exit_code=$exit_code" >> "$GITHUB_OUTPUT"


If you want to be consistent, you can replace all the echo lines with printf ... though it doesn't especially matter either way.

IsaacG · 2025-06-27T01:09:09Z

.github/workflows/configlet-sync.yml

+          token: ${{ secrets.GITHUB_TOKEN }}
+          title: "🚨 configlet sync --test found unsynced tests"
+          content-filepath: /tmp/sync-test-output.txt
+          issue-number: ${{ steps.find_issue.outputs.issue_number || '' }}


Will steps.find_issue.outputs.issue_number ever be unset?

ahans · 2025-06-27T06:26:26Z

.github/workflows/configlet-sync.yml

+        shell: bash {0}
+        run: |
+          configlet_raw_output="$(./bin/configlet sync --tests)"
+          exit_code=$?
+          printf "exit_code=%d\n" "$exit_code" >> "$GITHUB_OUTPUT"
+          printf "output<<EOF\n%s\nEOF\n" "$configlet_raw_output" >> "$GITHUB_OUTPUT"
+          printf "configlet output:\n%s\n" "$configlet_raw_output"


There's no need to turn off set -e via a custom shell entry. Bash under set -e can handle commands whose non-zero exit code is handled by the calling script and therefore not considered an unexpected failure that should lead to termination (which is what set -e is for). Just use some conditional statement such as if to use this. I believe here it would even increase readability, because instead of storing the raw exit code, you could save a boolean with a better name and use that below.

Suggested change

shell: bash {0}

run: |

configlet_raw_output="$(./bin/configlet sync --tests)"

exit_code=$?

printf "exit_code=%d\n" "$exit_code" >> "$GITHUB_OUTPUT"

printf "output<<EOF\n%s\nEOF\n" "$configlet_raw_output" >> "$GITHUB_OUTPUT"

printf "configlet output:\n%s\n" "$configlet_raw_output"

run: |

if configlet_raw_output="$(./bin/configlet sync --tests)"; then

printf "unsynced_tests_found=false\n" >> "$GITHUB_OUTPUT"

else

printf "unsynced_tests_found=true\n" >> "$GITHUB_OUTPUT"

printf "output<<EOF\n%s\nEOF\n" "$configlet_raw_output" >> "$GITHUB_OUTPUT"

fi

printf "configlet output:\n%s\n" "$configlet_raw_output"

This implies that the string "true" or "false" is better than 0 or 1 somehow. This is when m shell code. Shell exit codes are very well understood. This also adds more code and sets up additional outputs that may or may not be set. I'm not sure this approach is necessarily better or more readable.

Alternatively, you can split the handling of configlet's exit code and writing to $GITHUB_OUTPUT:

Suggested change

shell: bash {0}

run: |

configlet_raw_output="$(./bin/configlet sync --tests)"

exit_code=$?

printf "exit_code=%d\n" "$exit_code" >> "$GITHUB_OUTPUT"

printf "output<<EOF\n%s\nEOF\n" "$configlet_raw_output" >> "$GITHUB_OUTPUT"

printf "configlet output:\n%s\n" "$configlet_raw_output"

run: |

if configlet_raw_output="$(./bin/configlet sync --tests)"; then

unsynced_tests_found="false"

else

unsynced_tests_found="true"

fi

printf "unsynced_tests_found=%s\n" "$unsynced_tests_found" >> "$GITHUB_OUTPUT"

printf "output<<EOF\n%s\nEOF\n" "$configlet_raw_output" >> "$GITHUB_OUTPUT"

printf "configlet output:\n%s\n" "$configlet_raw_output"

This would add configlet's output to $GITHUB_OUTPUT even if it is not used, but for better readability would be acceptable (it's also what your current implementation does).

If you find the if ... then ... else ... fi too verbose, I think something like this instead would also count as idiomatic Bash code (the exception about set -e in conditionals holds here as well):

Suggested change

shell: bash {0}

run: |

configlet_raw_output="$(./bin/configlet sync --tests)"

exit_code=$?

printf "exit_code=%d\n" "$exit_code" >> "$GITHUB_OUTPUT"

printf "output<<EOF\n%s\nEOF\n" "$configlet_raw_output" >> "$GITHUB_OUTPUT"

printf "configlet output:\n%s\n" "$configlet_raw_output"

run: |

unsynced_tests_found="false"

configlet_raw_output="$(./bin/configlet sync --tests)" || unsynced_tests_found="true"

printf "unsynced_tests_found=%s\n" "$unsynced_tests_found" >> "$GITHUB_OUTPUT"

printf "output<<EOF\n%s\nEOF\n" "$configlet_raw_output" >> "$GITHUB_OUTPUT"

printf "configlet output:\n%s\n" "$configlet_raw_output"

Using $? is also perfectly readable and idiomatic bash 🙂

This implies that the string "true" or "false" is better than 0 or 1 somehow.

When used as a boolean in a GH actions workflow, it is better, because boolean exists as a datatype and anything else is coerced when used in a boolean expression (this is what the if:s in the subsequent steps are), see here. Directly saying true or false avoids this coercion.

Shell exit codes are very well understood.

Sure, but besides success being zero and non-success something else, it depends on the tool. Without knowing configlet, from this snippet alone I would not know that a non-zero exit code means there are unsynced tests. I would have to read the rest of the workflow file to learn that. Using a more descriptive key than exit_code does convey more information.

This also adds more code

More code is not necessarily worse if it's easy to read. Even somebody with little Bash experience would know what the if ... then ... else ... fi does. As an alternative, I suggested the option with the || shortcut that adds less code.

and sets up additional outputs that may or may not be set.

Not true. I replaced one output (exit_code) with another (unsynced_tests_found). The statement about maybe being set or not applies just as well to exit_code.

I'm not sure this approach is necessarily better or more readable.

I guess that depends on perspective. Ultimately it was just a suggestion.

My main point was that it is not necessarily to override GH's reasonable set -e default. That you can use more descriptive variable names in the process was an added benefit, IMHO at least.

(Edit: Many typos fixed.)

set -e as a reasonable default is also a matter of debate. See https://mywiki.wooledge.org/BashFAQ/105

ahans · 2025-06-27T06:27:12Z

.github/workflows/configlet-sync.yml

+
+      - name: Format test sync output and find existing issue
+        id: find_issue
+        if: ${{ steps.sync_test.outputs.exit_code != 0 }}


If you go with my suggestion, this would become:

Suggested change

if: ${{ steps.sync_test.outputs.exit_code != 0 }}

if: ${{ steps.sync_test.outputs.unsynced_tests_found }}

ahans · 2025-06-27T06:28:19Z

.github/workflows/configlet-sync.yml

+          fi
+
+      - name: Create or Update issue if tests are not synced
+        if: ${{ steps.sync_test.outputs.exit_code != 0 }}


Dito:

Suggested change

if: ${{ steps.sync_test.outputs.exit_code != 0 }}

if: ${{ steps.sync_test.outputs.unsynced_tests_found }}

ErikSchierboom

Let's wait with merging for the ongoing discussion on exit codes and booleans to wrap up

Create configlet-sync.yml

fd7edaa

Add configlet-sync.yml

This comment was marked as resolved.

Sign in to view

github-actions bot closed this Jun 24, 2025

BethanyG reopened this Jun 24, 2025

IsaacG reviewed Jun 24, 2025

View reviewed changes

.github/workflows/configlet-sync.yml Outdated Show resolved Hide resolved

.github/workflows/configlet-sync.yml Outdated Show resolved Hide resolved

IsaacG reviewed Jun 24, 2025

View reviewed changes

.github/workflows/configlet-sync.yml Outdated Show resolved Hide resolved

jagdish-15 and others added 5 commits June 24, 2025 18:50

Update .github/workflows/configlet-sync.yml

051bc0c

Co-authored-by: Isaac Good <[email protected]>

Update .github/workflows/configlet-sync.yml

c22a444

Co-authored-by: Isaac Good <[email protected]>

Update .github/workflows/configlet-sync.yml

32068b7

Co-authored-by: Isaac Good <[email protected]>

Update .github/workflows/configlet-sync.yml

e69b506

Co-authored-by: Isaac Good <[email protected]>

Update configlet-sync.yml to use temp dir

e9fa3bf

IsaacG reviewed Jun 24, 2025

View reviewed changes

jagdish-15 added 2 commits June 24, 2025 23:52

Update configlet-sync.yml

4fbaa98

Use /tmp file

07032b3

ErikSchierboom reviewed Jun 24, 2025

View reviewed changes

IsaacG reviewed Jun 24, 2025

View reviewed changes

.github/workflows/configlet-sync.yml Outdated Show resolved Hide resolved

.github/workflows/configlet-sync.yml Outdated Show resolved Hide resolved

kahgoh reviewed Jun 25, 2025

View reviewed changes

jagdish-15 and others added 7 commits June 26, 2025 23:36

Update .github/workflows/configlet-sync.yml

a2f337b

Co-authored-by: Isaac Good <[email protected]>

Update .github/workflows/configlet-sync.yml

e33554a

Co-authored-by: Isaac Good <[email protected]>

Pin actions/checkout to SHA

aea5758

Pin peter-evans/create-pull-request to SHA

200fa6b

Pin peter-evans/create-issue-from-file to SHA

9b402cd

Switch to exit_code verification

01e2caf

Switch to reusable

ed6d9f7

glennj reviewed Jun 26, 2025

View reviewed changes

.github/workflows/configlet-sync.yml Outdated Show resolved Hide resolved

Specify using bash {0}

a024c8a

Co-authored-by: Glenn Jackman <[email protected]>

IsaacG approved these changes Jun 27, 2025

View reviewed changes

jagdish-15 added 2 commits June 27, 2025 08:47

Remove STDERR part

820a7ac

Replace all echo with printf

240a3dd

ahans reviewed Jun 27, 2025

View reviewed changes

ErikSchierboom approved these changes Jun 27, 2025

View reviewed changes

	if: ${{ steps.sync_test.outputs.exit_code != 0 }}
	if: ${{ steps.sync_test.outputs.unsynced_tests_found }}

Uh oh!

Create configlet-sync.yml #212

Are you sure you want to change the base?

Create configlet-sync.yml #212

Conversation

jagdish-15 commented Jun 24, 2025

Pull Request

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikSchierboom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jagdish-15 commented Jun 26, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahans Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahans Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahans Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ErikSchierboom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahans Jun 27, 2025 •

edited

Loading

ahans Jun 27, 2025 •

edited

Loading

ahans Jun 27, 2025 •

edited

Loading