action.yml compliance

[chabad360]

This will make the image-actions action compliant with the current GitHub Actions action.yml format for describing actions.

This will also make the action easier to configure.

[benschwarz]

Hey @chabad360 - Introducing this change will break all existing config files silently.

To be able to action this we'd need a guide to explain how to make the change and ensure that we didn't break existing configs. Have you thought about how that change might be introduced?

What benefits are there in changing to actions.yml? It's neither required or enforced so the change seems semantic more than anything else.

[chabad360]

Hey @chabad360 - Introducing this change will break all existing config files silently.

Admittedly, I shouldn't have broken existing custom configs (in my defense, it would still just fall back to the existing defaults). I'll force-push non-breaking functionality in the near future.(I've since done that)

What benefits are there in changing to actions.yml? It's neither required or enforced so the change seems semantic more than anything else.

As to why? Well, using an action.yml is enforced for all new releases (I discovered this the hard way), it's also possible that GitHub may at some point get rid of all non-compliant actions. Also, the current form of configuration is counter-intuitive.

[lannonbr]

In GitHub's changelog, @chabad360 is correct that this is a requirement now: https://github.blog/changelog/2019-10-10-the-github-actions-marketplace-now-requires-an-actions-metadata-file/

[benschwarz]

Thanks for the heads up @lannonbr. We wrote the action long before actions.yml was a concept and GitHub didn't do a super job of alerting beta users to updates like this one.

We'll review this PR and move to get it merged. 👍

@chabad360, is this ready for review from your perspective?

[chabad360]

I think so. let me know if you find any problems.

…

On 12/2/19, Ben Schwarz ***@***.***> wrote: Thanks for the heads up @lannonbr. We wrote the action long before `actions.yml` was a concept and GitHub didn't do a super job of alerting beta users to updates like this one. We'll review this PR and move to get it merged. 👍 @chabad360, is this ready for review from your perspective? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #29 (comment)

-- Mendel G chabad360@gmail.com

[benschwarz]

I've left a bunch of questions, comments and required changes. Otherwise this doesn't seem too far off ready to go.

[chabad360]

I made some changes, have a look.

[pelikhan]

Is this a breaking change? Will existing actions fail or silently fail?

[chabad360]

No.

…

On 12/3/19, Peli de Halleux ***@***.***> wrote: Is this a breaking change? Will existing actions fail or silently fail? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #29 (comment)

-- Mendel G chabad360@gmail.com

[lannonbr]

The only thing is if someone uses the custom image-actions.yml file for the config of this action, it will no longer use those custom fields and fallback to the defaults unless they transfer to using the parameters inside the workflow using the with key. So I would maybe either add something in a changelog to explain how to upgrade, or include both the with and the image-actions.yml support for custom arguments

[chabad360]

The only thing is if someone uses the custom image-actions.yml file for the config of this action, it will no longer use those custom fields and fallback to the defaults unless they transfer to using the parameters inside the workflow using the with key.

It's actually the opposite, and if you were to use both image-actions.yml and the with key, it will use the former since the with key is set as the fallback.

This would similarly be the case if we implement what I suggest here (I'm strongly considering doing it).

Basically, I intend not to have any breaking changes with this PR, as that would be a really bad idea...

However, your point about documenting the change better (i.e. making it clear the there are no breaking changes) is true, and I will try to get to that tomorrow. Update: Done.

[benschwarz]

Thank you for your patience @chabad360 — I've left a final round of review.

By making these changes we're essentially deprecating the pre-existing configuration method, but it should lead to more clarity overall. 👍 👍 Well done! 🎉

[alexdupre]

BTW, most of the settings in image-actions.yml (and also the default set) are already ignored: #39

[chabad360]

I believe all is ready!

[benschwarz]

I created a test repo to take your branch for a spin, but it failed with a missing GITHUB_TOKEN.

Any thoughts @chabad360?

[benschwarz]

It looks as though github actions read the action.yml file from your branch, but then executed using the prebuilt (@master) docker image.

[chabad360]

It looks as though github actions read the action.yml file from your branch, but then executed using the prebuilt (@master) docker image.

Yup, that would definitely be why...
The reason for that is quite simple, my actions.yml file is set to use your image (obviously), but I've tested my changes here (Note that this also tests #30).

[benschwarz]

If you wouldn't mind the additional commits, it'd be useful to see if github are doing any docker caching. If not we'll have to continue to pre-build @master for image-actions releases. 😒

[chabad360]

I've just updated my previous comment (spoilers: use the yaml-test branch).

it'd be useful to see if github are doing any docker caching

To my knowledge, nope.

If not we'll have to continue to pre-build @master for image-actions releases.

Well, it's automatic anyways and is definitely the most practical option (better than caching, I think), but it would be nice if we could just use the GitHub Package Registry for that (sadly, we can't).

[chabad360]

Aaaaaand, it works!!!

[benschwarz]

Aaaaaand, it works!!!

Looks like we have a small issue to overcome - the YAML quality number arguments probably need to be parseInt'd before being pushed into sharp. (This is likely a preexisting issue, but I'd love to call this all done and ready!)

[chabad360]

Now it's working.

the YAML quality number arguments probably need to be parseInt'd before being pushed into sharp.

Yup, that was the issue.
Plus, the action should error out if Sharp has any more errors.

[benschwarz]

Thank you for your tireless work on this @chabad360. A huge improvement 🎉 !

[chabad360]

Yay!

[chabad360]

Now on to #30!

[alexdupre]

If you want to deprecate the old config file I'd say that the new way to specify settings should support all previous settings. Eg. creating progressive jpeg is very important for web images, and the only way to do it currently is via the old config file that is not documented anymore.

[chabad360]

@alexdupre How did one configure progressive JPEGs?

[chabad360]

@alexdupre What do you think of #44?

[alexdupre]

Not tested, but looks fine.