Skip to content
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

Converted actions to YAML syntax #5

Merged
merged 2 commits into from
Oct 7, 2019
Merged

Conversation

manicki
Copy link
Member

@manicki manicki commented Sep 27, 2019

@manicki
Copy link
Member Author

manicki commented Sep 27, 2019

c781cae is a result of running the recommended migration script but it does look wrong at glance (dependencies between action seems to have got missed).
Also the build failure shows the automated migration didn't really work: https://github.com/wmde/vuex-helpers/runs/238623339

@wiese
Copy link
Contributor

wiese commented Sep 27, 2019

It does not look like it went horribly wrong - just the tagging funnel seems to be overreacting. Don't have time to research but maybe - given this is still somewhat in beta - something really is not quite right about interpreting the exit code ("##[error]Docker run failed with exit code 78") in this case.

@manicki
Copy link
Member Author

manicki commented Sep 30, 2019

Yup, looks like this was about handling the exit code: https://github.com/actions/migrate/issues/42. I've followed the advice given there and dropped the filtering step, and included filtering in the Publishing step.
I've just copied code from somewhere, and am not at all sure whether it is the best approach. Among other things, I am not sure why/if "created" is a better event type to filter by than "published". https://help.github.com/en/articles/events-that-trigger-workflows#release-event-release is not very informative on this.

Also it looks the repo branch protection settings would need to be changed before merging this as there is no longer "Test" action.
Actually, I am not even convinced the single "Build" action with multiple steps like the tool has generated it is the best structure for defining those workflows.

Copy link
Contributor

@wiese wiese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging this up.

As you mentioned we are (very much so) witnessing a product under development - I'd be ok with accepting the spotty doc and go with the filter rules you dug up; especially as "release created" does not sound totally unintuitive.

That said, the announcement puzzles me, especially as act totally is not ready for it (nektos/act#74 / nektos/act#79) but with the "old" syntax being deprecated today, let's do this and find out if it actually works.

As a perk (compared to before) this filter does now seem to get applied before a container is spun up (try and expand the filter step in the build log) so you are also paying into our environmental sustainability goals.

Am sad to see workflow rendering go this way but maybe it will make a return.

FTR: did not touch the branch protection

@manicki manicki merged commit 2025a88 into master Oct 7, 2019
@manicki manicki deleted the migrating-to-yaml-syntax branch October 7, 2019 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants