-
Notifications
You must be signed in to change notification settings - Fork 2
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
ci: workflows for local runs and GH actions #18
Conversation
Reviewer's Guide by SourceryThis pull request introduces initial CI/CD workflows for both local and GitHub Actions. The local.yml workflow includes caching mechanisms for various dependencies to speed up local testing, while the ci.yml workflow is designed for continuous integration on push and pull request events without caching. Both workflows set up necessary environment variables, install required dependencies (Rust, JDK 11, Go, Funnel), and include steps for building models, running tests, linting, and formatting. Note that the tests currently fail due to the absence of the build-models.sh script, which is addressed in a separate pull request. File-Level Changes
Tips
|
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.
Hey @aaravm - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
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.
LGTM!
.github/workflows/ci.yml
Outdated
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 might be useful to define separate jobs for running tests, linting and formatting checks. That way they can run in parallel, and it is easier to see at a glance where a problem arose. See here for an example.
Of course, for each job you only need to install what is really required for each job (e.g., JDK and Funnel will likely not be needed for linting and formatting).
Btw, this workflow also highlights the issues that may quickly arise when writing your unit/integration tests in a way that they depend on other services. Not only running the tests, but also setting them up (e.g. installing Rust, Go, JDK - and the list will quickly get longer the more services you require for testing) will take increasingly more time.
I think it is important to consider and implement a scalable testing strategy early, e.g.:
- Use multiple GitHub Actions workflows for static code analysis (e.g., linting, formatting, code quality), different test types (see below) and other checks (PR validation, security etc.); again, see here for an example
- Keep unit and basic integration tests dependency free (by monkey patching and mocking expected responses), so as to keep the turnaround time for tests low during regular development.
- Define potentially long-running end-to-end tests to comprehensively test client functionalities against real world reference implementations for each client (e.g., Funnel for TES); only run those tests when staging releases - in a separate GitHub Actions workflow.
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.
Minus the caching (which would also be useful when not triggered locally, no?), this is essentially the same workflow as the previous one. Couldn't you just add workflow_dispatch
as a trigger to the first workflow?
It is a considerable maintenance burden to keep multiple copies of (virtually) the same code in sync - sooner or later they will diverge!
Also, please document (e.g., in README.md
) how to trigger workflow execution locally.
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.
I've just seen that triggering the workflow locally is documented in #29, so I guess you can ignore that.
It would of course be nice to ensure that the workflows runs successfully before merging. Maybe this will be the case if the PRs are merged in the right order... |
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.
Perhaps better to extend #34 in favor of this PR? Looks more mature/complete.
#34 is now doing the CI/CD |
This branch contains the initial continuous integration in 2 files: local.yml (which includes caching, allowing for faster local testing) and ci.yml, which doesn't include caching.
Also, note that the tests currently fails since build-models.sh has not been added to this branch. It is added in another PR
Summary by Sourcery
Introduced initial continuous integration workflows for local and GitHub CI/CD, including dependency caching for local testing and basic setup for GitHub actions.