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

Migrate to Github Actions #351

Merged
merged 11 commits into from
Feb 1, 2021
Merged

Migrate to Github Actions #351

merged 11 commits into from
Feb 1, 2021

Conversation

kanekosh
Copy link
Contributor

@kanekosh kanekosh commented Jan 30, 2021

Purpose

Migrate CI/CD tool from Travis to Github Actions.

  • oas_pr.yml is triggered when a PR is created. This runs tests and builds docs (but does not deploy it).
  • oas_push.yml is triggered when a PR is merged to master (i.e., pushed to master). In addition to the tests and doc builds, it deploys the docs to gh-pages.

Type of change

What types of change is it?

  • Other (maintenance update)

Checklist

  • I have run unit and regression tests which pass locally with my changes

@kanekosh
Copy link
Contributor Author

Just for future reference:
This extension: https://github.com/marketplace/actions/sphinx-build looked helpful, but for some reason it required re-installing everything (openaerostruct, openmdao, mpi4py etc...) in its own block using pre-build-command.
Since these packages are already installed in earlier blocks (for tests), I decided to not use it and directly build the docs via make html.

@coveralls
Copy link

coveralls commented Jan 30, 2021

Coverage Status

Coverage remained the same at 96.6% when pulling 866c1c0 on gh_actions into b36c186 on master.

@kanekosh kanekosh marked this pull request as ready for review January 30, 2021 01:54
@kanekosh kanekosh requested a review from a team as a code owner January 30, 2021 01:54
@kanekosh kanekosh requested review from ewu63, shamsheersc19 and bernardopacini and removed request for shamsheersc19 January 30, 2021 01:54
@kanekosh
Copy link
Contributor Author

I believe it's working now. If it looks good, I will remove the travis file before merging.
It looks to me that GH Actions is properly blocking the merge (as travis was doing so), but I didn't any settings for that, so I am a bit worried.

Copy link
Collaborator

@ewu63 ewu63 left a comment

Choose a reason for hiding this comment

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

Looks fine. My main question is whether it's possible to combine these into a single file. I don't like the code duplication in two files that are largely the same.

.github/workflows/oas_pr.yml Outdated Show resolved Hide resolved
@ewu63 ewu63 linked an issue Jan 31, 2021 that may be closed by this pull request
@kanekosh
Copy link
Contributor Author

kanekosh commented Feb 1, 2021

Looks fine. My main question is whether it's possible to combine these into a single file. I don't like the code duplication in two files that are largely the same.

Agreed, will take a look, I believe it should be possible.

@ewu63
Copy link
Collaborator

ewu63 commented Feb 1, 2021

One last thing: I believe we want coveralls to run on the PR build also, so that we can monitor the testing coverage and make sure it doesn't decrease by much. On Travis, the after_success stage is run regardless of whether it's a push to master or PR build, and I guess travis-sphinx deploy just failed on PR build since it requires a secret token (set on Travis website) to deploy.

After this is merged I will update the branch protection rule to switch the required status checksfrom Travis to GitHub Actions.

@kanekosh
Copy link
Contributor Author

kanekosh commented Feb 1, 2021

I believe it's good to go! I unified two files and added if statements so that docs deployment are only called when we push to the master branch. Coveralls runs on both PR builds and merge builds. I tested the if statements here and it worked.

I also removed the Travis file.

@ewu63
Copy link
Collaborator

ewu63 commented Feb 1, 2021

Awesome stuff! Sorry to be picky but I really wanna make sure we're careful here. Did you ever test that this is working as expected? i.e. push a commit that deliberately broke something and check that GHA fails.

EDIT: I guess that's not super relevant here but with Docker-based builds there was a time where we never checked out the PR, so we were always just testing the Docker image which always passed. I'm happy to merge this as is.

@kanekosh
Copy link
Contributor Author

kanekosh commented Feb 1, 2021

I deliberately broke one of the tests, and now GHA fails as it is supposed to.
The documentation build is a bit tricky, though. GHA fails when make html fails, for example when we didn't specify the correct dependencies on requirements.txt. However, when make html finishes with some warnings, GHA does not detect and block that. This happens when embedded python scripts have some errors. In such a case, the docs do have embedded code on them, but since python execution failed, we don't have the python output (like optimization result printout) on the docs.

I am not sure if we can deal with this at the GHA level.

@ewu63
Copy link
Collaborator

ewu63 commented Feb 1, 2021

Yeah that seems like something to figure out with the OpenMDAO toolchain for building docs. We can investigate that in the future. Happy to merge now, just a reminder to make a release after.

@ewu63 ewu63 merged commit 5d03d05 into master Feb 1, 2021
@ewu63 ewu63 deleted the gh_actions branch February 1, 2021 04:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to GitHub Actions
3 participants