-
-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First pass at removing set-output
#235
Conversation
`set-output` has been deprecated. See https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/.
grin @desrosj Only now seeing your PR, I was working on the same thing ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I haven't tested this, the code changes look good (save for the composer.lock
file changes which need to be undone).
bin/composer_paths.sh
Outdated
echo "::set-output name=cache-dir::${cache_dir}" | ||
echo "::set-output name=json::${composer_json}" | ||
echo "::set-output name=lock::${composer_lock}" | ||
echo "command=${composer_path}" >> "${GITHUB_OUTPUT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shellcheck doesn't like this output being called command
...
In bin/composer_paths.sh line 56:
echo "command=${composer_path}" >> "${GITHUB_OUTPUT}"
^-- SC2129: Consider using { cmd1; cmd2; } >> file instead of individual redirects.
For more information:
https://www.shellcheck.net/wiki/SC2129 -- Consider using { cmd1; cmd2; } >>...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the variable name to composer_command
.
It doesn't seem like this will have any backwards compatibility concerns since it's only used within the repo. But not sure if I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd need to check, but this output may also be available to steps in external workflows being run after the step using this action runner.
If that's the case and while it would be rare for a workflow to use the command
output, it would still need a changelog entry. Though as I said, this would need to be checked and confirmed first.
lock=./composer.lock\n | ||
" | ||
|
||
if { $expectedValue != $fileData } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails as it needs to do a regex compare, not a direct comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bash scripting is admittedly poor. Do you have an example I could use as a guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bash scripting skills are even worse, then again, my Google skills are reasonable ;-)
- Looks like a wildcard compare is possible, but the pattern used here is probably too complex for that.
- This guide about pattern matching and regex in bash looks very helpful.
- And this one may be of use as well.
lock=\r | ||
" | ||
|
||
if { $expectedValue != $fileData } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails as it needs to do a regex compare, not a direct comparison.
lock=../fixtures/with-lock-file/composer.lock\r | ||
" | ||
|
||
if { $expectedValue != $fileData } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails as it needs to do a regex compare, not a direct comparison.
lock=../fixtures/out-of-sync-lock/composer.lock\n | ||
" | ||
|
||
if { $expectedValue != $fileData } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails as it needs to do a regex compare, not a direct comparison.
lock=./composer.lock\r | ||
" | ||
|
||
if { $expectedValue != $fileData } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails as it needs to do a regex compare, not a direct comparison.
\n | ||
" | ||
|
||
if { $expectedValue != $fileData } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails as it needs to do a regex compare, not a direct comparison.
Bash gets confused with the variable being named `command`.
I've addressed a handful of the feedback given. I also added some brief inline documentation to the tests. |
@@ -27,5 +33,19 @@ if { $expectedValue != $fileData } { | |||
exit 1 | |||
} | |||
|
|||
# Confirm environment variables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Confirm environment variables. | |
# Confirm output variables. |
(here and in the other test files)
Thank you! I've been away for a bit, but I'll take a look at this today. |
I won't have a chance to circle back to this until Wednesday. @ramsey feel free to make any changes to wrap this up if you'd like to get it out sooner! |
I saw the build errors in #237 but assumed they'd go away when merged into this PR. I'll try to find some time this week to see if I can help resolve things to get this merged and tagged. Thanks! |
They will (should) once the strict comparisons have been changed to regex/wildcard-based comparisons for the tests I flagged up above ;-) |
Anything I can do to help move this forward ? |
I haven't had a chance to circle back to this because of a few other things going on at the moment. I'll likely have time next week, but if you'd like to figure out the needed test adjustments, I'm more than happy to hand the rest of this over to someone else in the interest of seeing this fixed/released. |
Okay, so I've just spend a couple of hours trying to get this to work and in the end I ended up in the Tcl manual and it looks like I may have actually managed to get it working... I need to run some more tests to be sure, but I'm hoping to send in a PR later today combining @desrosj commits from this PR with the fix I found. |
Wowie! Tested (including deliberately breaking tests to make sure the solution will correctly fail tests when needed) and I now have a 🟢 build. Will clean up my WIP branch and pull it momentarily. |
I've opened PR #238 which builds onto this one and gets us to a green CI build again. |
Work in progress. But feedback is welcome.
Description
set-output
has been deprecated. See https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/.Motivation and context
This will remove
deprecated
notices in workflow summaries.How has this been tested?
Types of changes
PR checklist