First pass at removing `set-output`

ramsey / composer-install

:gift: A GitHub Action to streamline installation of PHP dependencies with Composer.

MIT License

227 stars 32 forks source link

First pass at removing `set-output` #235

Closed desrosj closed 1 year ago

desrosj commented 1 year ago

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

[x] 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.
[x] I have read the CONTRIBUTING.md document.
[ ] I have added tests to cover my changes.

jrfnl commented 1 year ago

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

desrosj commented 1 year ago

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

jrfnl commented 1 year ago

@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

ramsey commented 1 year ago

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

desrosj commented 1 year ago

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!

ramsey commented 1 year ago

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!

jrfnl commented 1 year ago

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 ;-)

jrfnl commented 1 year ago

Anything I can do to help move this forward ?

desrosj commented 1 year ago

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.

jrfnl commented 1 year ago

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.

jrfnl commented 1 year ago

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 commented 1 year ago

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