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.

Sign up for GitHub

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

Jump to bottom

First pass at removing `set-output` #235

Closed

desrosj wants to merge 7 commits into ramsey:v2 from desrosj:remove/set-output

Contributor

desrosj commented Oct 14, 2022

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

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

PR checklist

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING.md document.
I have added tests to cover my changes.


          First pass at removing set-output.

d71060f

`set-output` has been deprecated. See https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/.

Contributor

jrfnl commented Oct 14, 2022

grin @desrosj Only now seeing your PR, I was working on the same thing ;-)

jrfnl mentioned this pull request

set-output function is deprecated #236

Closed

jrfnl suggested changes

View reviewed changes

Contributor

jrfnl left a comment

While I haven't tested this, the code changes look good (save for the composer.lock file changes which need to be undone).

tests/expect/composer.lock Outdated Show resolved Hide resolved

tests/fixtures/with-lock-file/composer.lock Outdated Show resolved Hide resolved

jrfnl reviewed

View reviewed changes

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}"

Contributor

jrfnl Oct 14, 2022

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; } >>...

Contributor Author

desrosj Oct 14, 2022 •

edited

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.

Contributor

jrfnl Oct 14, 2022

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.

jrfnl reviewed

View reviewed changes

tests/expect/composer_paths_01.exp

+              lock=./composer.lock\n
+              "
+              if { $expectedValue != $fileData } {

Contributor

jrfnl Oct 14, 2022

This test fails as it needs to do a regex compare, not a direct comparison.

Contributor Author

desrosj Oct 14, 2022

My bash scripting is admittedly poor. Do you have an example I could use as a guide?

Contributor

jrfnl Oct 14, 2022

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.

jrfnl reviewed

View reviewed changes

tests/expect/composer_paths_05.exp

+              lock=\r
+              "
+              if { $expectedValue != $fileData } {

Contributor

jrfnl Oct 14, 2022

This test fails as it needs to do a regex compare, not a direct comparison.

jrfnl reviewed

View reviewed changes

tests/expect/composer_paths_06.exp

+              lock=../fixtures/with-lock-file/composer.lock\r
+              "
+              if { $expectedValue != $fileData } {

Contributor

jrfnl Oct 14, 2022

This test fails as it needs to do a regex compare, not a direct comparison.

jrfnl reviewed

View reviewed changes

tests/expect/composer_paths_08.exp

+              lock=../fixtures/out-of-sync-lock/composer.lock\n
+              "
+              if { $expectedValue != $fileData } {

Contributor

jrfnl Oct 14, 2022

This test fails as it needs to do a regex compare, not a direct comparison.

jrfnl reviewed

View reviewed changes

tests/expect/composer_paths_09.exp

+              lock=./composer.lock\r
+              "
+              if { $expectedValue != $fileData } {

Contributor

jrfnl Oct 14, 2022

This test fails as it needs to do a regex compare, not a direct comparison.

jrfnl reviewed

View reviewed changes

tests/expect/php_version_01.exp

+              \n
+              "
+              if { $expectedValue != $fileData } {

Contributor

jrfnl Oct 14, 2022

This test fails as it needs to do a regex compare, not a direct comparison.

desrosj added 5 commits

October 14, 2022 09:29


          Revert unintentional lock file change.

d0a2664


          Change command to composer_command.

ed70145

Bash gets confused with the variable being named `command`.


          Undo composer.lock file changes within test fixtures.

b687a69


          Add some inline documentation to tests.

58f0a95


          Change missed command instance to composer_command.

0835a07

Contributor Author

desrosj commented Oct 14, 2022

I've addressed a handful of the feedback given. I also added some brief inline documentation to the tests.

jrfnl reviewed

View reviewed changes

tests/expect/cache_key_01.exp

@@ @@ -27,5 +33,19 @@ if { $expectedValue != $fileData } { @@
                   exit 1
               }
+              # Confirm environment variables.

Contributor

jrfnl Oct 14, 2022

Suggested change

      
            # Confirm environment variables.
          
            # Confirm output variables.

(here and in the other test files)

Contributor

jrfnl commented Oct 22, 2022

@ramsey Would you happen to be able to give some guidance ? Would be nice if the CI related PRs (this one and #237) could get merged soonish, so I can pull the PR for the feature request in #234

Owner

ramsey commented Oct 22, 2022

Thank you! I've been away for a bit, but I'll take a look at this today.


          Merge branch 'v2' into remove/set-output

b7f48f9

Contributor Author

desrosj commented Oct 24, 2022 •

edited

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!

Owner

ramsey commented Oct 24, 2022

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!

Contributor

jrfnl commented Oct 24, 2022

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.

They will (should) once the strict comparisons have been changed to regex/wildcard-based comparisons for the tests I flagged up above ;-)

Contributor

jrfnl commented Nov 2, 2022

Anything I can do to help move this forward ?

Contributor Author

desrosj commented Nov 3, 2022 •

edited

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.

Contributor

jrfnl commented Nov 3, 2022

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.

Contributor

jrfnl commented Nov 3, 2022

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.

jrfnl mentioned this pull request

Fix use of deprecated set-output command #238

Merged

7 tasks

Contributor

jrfnl commented Nov 3, 2022

I've opened PR #238 which builds onto this one and gets us to a green CI build again.

ramsey closed this in #238

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment