Refactor/GitHub workflow

[harrysarson]

Opening this PR to check the on: pull_request workflow.

[josdejong]

I've added the browserstack keys to the secrets section of github, all works like a charm. I really like the neat separation in files to run on push vs pull_request.

Should the build.yaml file also run on pull_request?

[harrysarson]

Should the build.yaml file also run on pull_request?

Yes it should.

[josdejong]

Thanks for the update Harry.

I'm wondering, it looks like all tasks of build.yml are executed twice: once for push and once for pull_request. Any idea how we can prevent that from happening?

And I guess the "continuous-integration/travis-ci" check is a left over and will be gone after we've merged all.

[harrysarson]

I think that happens because this is both a branch (in the mathjs repo) and a pull request (against the mathjs repo). This would not happen if someone makes a pull request from a fork this workflow would not run twice.

I am not sure if we can stop it running twice for branches that are also pull requests.

[josdejong]

Hm. So maybe when you update a PR, push is also triggered? Would it be enough to just trigger build.yml on push after all then, and not both?

[harrysarson]

Would it be enough to just trigger build.yml on push after all then

Not if we want the workflow to run when someone makes a pull request from a fork

[josdejong]

Hm, I guess we have too keep it like this though. It's similar to travis where we also had this, right? (only you didn't see this in a single list with all tasks).

So I think we can merge this PR, right @harrysarson ?

[harrysarson]

Hm, I guess we have too keep it like this though.

Currently:

on: [push, pull_request]

We could:

on:
  push:
    branches:
      - develop
      - master
    tags:  
  pull_request:

because any other branch would end up being a pull_request so the build job would get run then.

It's similar to travis where we also had this, right? (only you didn't see this in a single list with all tasks).

Yes, although running less CI jobs is always a good thing if we can do it!

[josdejong]

I think the branches: configuration makes sense, we can try it out. What do you think?

[harrysarson]

Oops, I changed the wrong file! Let me fix next time I get to my laptop 😊

[harrysarson]

Do check when this is merged into dev that the build job runs :)

Is it intentional that we do not run CI on nodejs 10?

[josdejong]

Do check when this is merged into dev that the build job runs :)

Is it intentional that we do not run CI on nodejs 10?

Ahhh, wait a second. In the old Travis script we have this part:

mathjs/.travis.yml

Lines 14 to 16 in 59aacb4

	- script: npm run test:src:transpile
	node_js: 10
	name: Node 10

So I think my former me made this test:src:transpile script specially for nodejs 10, which is missing some of the newer JavaScript features and requires transpilation before you can run the tests. Argh. So I guess we'll have to add this script again and a Github workflow for it, until end-of-life of nodejs 10, which is coming April 2021.

[harrysarson]

Take a look! We now run special script on node 10. I also tidied the scripts up nicely.

[josdejong]

Looks good, nicely solved with the node 10 script, having all logic for this in one place 👍 .

Ready to merge I think?

[harrysarson]

fixes #2024

[harrysarson]

ready to merge I think

[josdejong]

👍 thanks again!

[harrysarson]

Is good to be back! Merry Christmas Josh, till next time.

[josdejong]

It works like a charm, this is awesome 😎

Happy holidays Harry!