SuffolkLITLab / ALKiln

Integrated automated end-to-end testing with docassemble, puppeteer, and cucumber.
https://assemblyline.suffolklitlab.org/docs/alkiln/intro
MIT License
14 stars 4 forks source link

Action: Remove need for repeating `shell: bash` #528

Closed plocket closed 2 years ago

plocket commented 2 years ago

illegitimate nitpick: it is out-of-scope of this PR, but I believe https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#defaultsrun will remove the need of repeating the shell: bash.

_Originally posted by @k3KAW8Pnf7mkmdSMPHz27 in https://github.com/SuffolkLITLab/ALKiln/pull/527#discussion_r833608381_

BryceStevenWilley commented 2 years ago

I'm actually convinced that the default shell in the Github Actions is bash (https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsshell), but the things that we can't actually control is that the default in npm run ... is shell. Changing the npm default makes things not usable in windows locally though. Some more discussion in #540.

So, fairly certain that just removing shell: bash would be fine, we don't need to use the defaults.

plocket commented 2 years ago

I keep trying to remove shell: bash in various places, like our action.yml, and keep running into errors.

BryceStevenWilley commented 2 years ago

What are the errors? The one place I can find you having to add it back, there were other problems with the action.yml syntax (which you fixed in a subsequent commit, specifically using run: | instead of run:). I can't actually see what was going wrong in that commit though.

plocket commented 2 years ago

Good eye, so I tried it again and still got an error that shell was required:

BryceStevenWilley commented 2 years ago

Hm, I see. Looking into it more, shell is required when run is set in a step, specifically in action.yml / composite actions. So I'm not sure the original change or my suggestion are possible in the action.yml, and we already don't specify shell in our workflow file. Apologies, I didn't realize that workflow files and action.yml files have such different rules.

I suggest we close this issue.

plocket commented 2 years ago

If we've chosen to drop support for developing in Windows, we can use default, right? Is it worth it?

BryceStevenWilley commented 2 years ago

1) not sure we can use default? That would be invalid yaml, as composite actions specifically say that the shell param is required, making me think that we need to specify bash. If we are allowed to use default, give default does almost exactly the same thing as bash on Ubuntu runners, I don't really think it's worth the change to be more vague about which shell is running. 2) I don't think Windows support matters here, as the workflow runners will be Ubuntu by default, and I don't think most people will want to change them to Windows (if they do, they can still use a separate Ubuntu workflow for just Kiln stuff).

plocket commented 2 years ago

Closing because it turns out we do need to declare shell for every run.