-
-
Notifications
You must be signed in to change notification settings - Fork 726
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
Fritz hoing Github actions improvements #6120
Conversation
✅ Deploy Preview for plone-components canceled.
|
Closing in favor of having commit suggestions to the original pull request #6108 (review) |
…nd crash without it
@stevepiercy Have another look here, this pull request still contains the work of @FritzHoing with the right attribution plus my changes since it's very cumbersome todo suggest commits especially where it's a trial and error. |
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.
Thanks for the thorough verification of extracting all the repetitive steps, including bash requirement. I especially like that you added a name to an unnamed step. It's documentation! 😁
@plone/volto-team this is good work that you could use as an example and apply to your own CI to avoid copy-pasta mistakes. Please take a moment to review.
@ichim-david @FritzHoing Awesome work! Thanks for working on this. Could we find a better naming for the action? |
I find naming a thing by what it does, not its tool name, is usually more descriptive and long-lived. If you ever change the tool (which never happens in JavaScript), you don't have to change the name. It creates a universal test setup, so |
@sneridagh @stevepiercy I agree on the naming needs tweaking, it's for that reason that I replaced Pretest-Setup with I don't mind the universal_test_setup naming. Maybe you can come to an agreement on the naming and proposing it then, maybe talk with Eric and David and see what you guys thing makes most sense. |
@stevepiercy @ichim-david the fact is that it can be used outside acceptance, in unit and code quality too. It's setting up the node version and pnpm in the right way. Also, is not universal, as we can configure other things too, like Python... |
@stevepiercy and I, we discussed it and agreed on |
@sneridagh I'm fine with it what about the step name? |
@ichim-david |
Yup. We have a winner! |
@sneridagh done, let's see the testing and then a merge of main and I think it's good to go. |
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.
It's Node.js per https://nodejs.org/en
Co-authored-by: Steve Piercy <web@stevepiercy.com>
* main: Release 18.0.0-alpha.37 Release @plone/types 1.0.0-alpha.17 fix(teaser): reset button undefined when no target (#6121) (feat): Add loading visual and succes message in controlpanel when deleting users and groups (#6127) Upgrade `react-intl` to maximum 3.x series. (#6128) Added three missing German translations. (#6124) Fritz hoing Github actions improvements (#6120)
📚 Documentation preview 📚: https://volto--6120.org.readthedocs.build/
Checked out pull request https://github.com/plone/volto/pull/6108/files and fixed the issues
regarding the action run error codes for example:
Error: /home/runner/work/volto/volto/./.github/actions/pre_test_setup/action.yml (Line: 4, Col: 3): There's not enough info to determine what you meant. Add one of these properties: args, entrypoint, env, image, main, post, post-entrypoint, pre, pre-entrypoint, steps