Skip to content
This repository has been archived by the owner on Aug 17, 2021. It is now read-only.

CI: improve workflow #100

Merged
merged 9 commits into from
Jul 25, 2020
Merged

CI: improve workflow #100

merged 9 commits into from
Jul 25, 2020

Conversation

tatchi
Copy link
Collaborator

@tatchi tatchi commented Jun 8, 2020

This gets rid of the bash scripts which block #93 and generally improve the whole CI process (I think). This is mostly based on redemon. Here is a general overview of the steps:

1. build_and_test

  • Build and test the project
  • Create a release on each platform with the esy npm-release command (which works now)
  • Upload the created release files

2. prepare-publish

  • Download the release files for each platform and create a single "package". It uses the bundle-release.js script from hello-reason template
  • Upload it

3. test-platform

  • Install the package produced in 2 and test it for each platform. It just runs a --help command but at least we can make sure that package can be installed and run correctly.

4. publish

  • Create a release and publish on NPM. Only when a tag is pushed (which is how it is currently working if I'm not mistaken).

Note that for now, the test-platform fails because the --help command returns an exit code 2 (while it works correctly). I tested it with #93 and it should be fixed (I guess due to one of the dependencies update)

esy.json Show resolved Hide resolved
esy.json Show resolved Hide resolved
@tatchi tatchi requested a review from dylanirlbeck June 8, 2020 16:50
Copy link
Owner

@dylanirlbeck dylanirlbeck left a comment

Choose a reason for hiding this comment

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

Overall, I'm liking these changes. Actually installing the PPX on each platform and running tailwind-ppx --help is a straightforward way to have confidence we did everything correctly. I have a few questions that I left in the comments, but mostly it looks good!!

Do we want to remove scripts/release-make-skeleton.js? It seems that you new bundle-release.js file effectively replaces it.

Finally, I see some of the test and install workflows are failing in CI. Can you take a look at this? I assume I'll be changing around which ones are required for pull requests, but let's just make sure everything is passing before merging in.

.github/workflows/pipeline.yml Show resolved Hide resolved
.github/workflows/pipeline.yml Outdated Show resolved Hide resolved
.github/workflows/pipeline.yml Show resolved Hide resolved
.github/workflows/pipeline.yml Show resolved Hide resolved
@tatchi
Copy link
Collaborator Author

tatchi commented Jun 15, 2020

Do we want to remove scripts/release-make-skeleton.js? It seems that you new bundle-release.js file effectively replaces it.

I removed it. Should we get rid of the release-postinstall.js one we well ?

Finally, I see some of the test and install workflows are failing in CI. Can you take a look at this? I assume I'll be changing around which ones are required for pull requests, but let's just make sure everything is passing before merging in.

Yes, that's normal. As explained, the --help command currently returns with an exit code 2 on Linux and macOS. This makes the CI step to fail. It should be fixed with #93. So once this one is merged, I will rebase #93 and everything should be ok (hopefully)

@tatchi tatchi mentioned this pull request Jun 23, 2020
@tatchi
Copy link
Collaborator Author

tatchi commented Jun 24, 2020

@dylanirlbeck I rebased on master now that #83 is merged and CI is green :)

Copy link
Owner

@dylanirlbeck dylanirlbeck left a comment

Choose a reason for hiding this comment

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

Overall, LGTM! I'm definitely ready to merge these changes in. Just three questions from me:

  1. Is there a way you can test that the use-tailwind-ppx script works on each platform? In the README I listed the different ways to call the script, so perhaps we could throw in a check there? I just want to make sure nothing is broken with the new flow.

  2. We do not want to remove the release-postinstall.js script, because that's what gets copied into the release zip folder and is run when a user installs the NPM package.

  3. After we merge this, which GitHub actions checks should be required on PRs? It looks like every check except the (only on release) Publish one, right?

Looking forward to merging this soon!

@tatchi
Copy link
Collaborator Author

tatchi commented Jul 22, 2020

  1. Is there a way you can test that the use-tailwind-ppx script works on each platform? In the README I listed the different ways to call the script, so perhaps we could throw in a check there? I just want to make sure nothing is broken with the new flow.

I added that to the CI config. A new BS project is bootstrapped where the use-tailwind-ppx command is run. Nothing is actually happening since there's nothing to convert but at least we make sure that the command is available on each platform.

  1. We do not want to remove the release-postinstall.js script, because that's what gets copied into the release zip folder and is run when a user installs the NPM package.

I think we can remove it. I use another one for the CI stuff now (taken from the hello-reason template). So except if you use it elsewhere, it should be ok to delete it.

  1. After we merge this, which GitHub actions checks should be required on PRs? It looks like every check except the (only on release) Publish one, right?

Yes indeed :) The publish one should get triggered when you create a tag.

@tatchi tatchi requested a review from dylanirlbeck July 22, 2020 19:19
@dylanirlbeck dylanirlbeck merged commit 16013fc into master Jul 25, 2020
@dylanirlbeck dylanirlbeck deleted the improve-ci branch July 25, 2020 17:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants