-
Notifications
You must be signed in to change notification settings - Fork 313
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
[Resolves #1401] Move CI to github actions #1412
Conversation
Move the Sceptre CI from using circleci to github actions. This also adds the usage of Github OIDC access to AWS to allow the execution of integration tests against an AWS account without providing github with user credentials. depends on Sceptre/sceptre-aws#14
.github/workflows/main.yaml
Outdated
role-session-name: GHA-${{ github.repository_owner }}-${{ github.event.repository.name }}-${{ github.run_id }} | ||
role-duration-seconds: ${{ env.AWS_ROLE_DURATION }} | ||
- name: run tests | ||
run: poetry run behave --junit --junit-directory build/behave |
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.
on circleci we split integration tests using the circle ci runner so that they run in parallel on multiple instances. I'm not sure how to replicate that in GH actions but i think it will be necessary because these tests take along time to complete. In parallel mode, they were taking 30mins to complete on circleci.
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's probably worth testing to see how long they take on the GitHub runners. Probably not much quicker, if at all, but I wouldn't prematurely optimize.
That said, if I were attempting this I would do two things:
- Break this out into a separate workflow -- this part isn't absolutely necessary, but will improve readability. And means you don't need the
if:
at the top. - Use a matrix build, where the matrix variables are test names. Then your command here would add
--include ${{ matrix.tests }}
.
Also, behave
has a --parallel
option. Could that help out here?
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's probably worth testing to see how long they take on the GitHub runners. Probably not much quicker, if at all, but I wouldn't prematurely optimize.
Good idea. Let's do the simple and, if needed, optimize on a follow on PR. I'm not sure running in separate workflows or as a matrix would work though because there are lots of dependencies between cloudformation stacks in those tests.
Also, behave has a --parallel option. Could that help out here?
Behave issue #235 indicates that it's there but it doesn't work.
While the behave that is present in behave-parallel, doesnt have that support.
And behave 1.2.5 doesnt have "processes" parameter
support (https://github.com/hugeinc/behave-parallel/blob/upstreamsync/behave/configuration.py#L71)
Also I tested that as an option when setting up split tests in circleci and it didn't really help at all.
🎉 All dependencies have been resolved ! |
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.
Overall, seems good! Unfortunately we won't be able to see if it actually works until this is merged. Though there is a way around that. Open another PR to add the .github/workflows/main.yaml
file, but keep it super basic. Just something like
name: main
on:
pull_request:
branches:
- '*'
In theory, if that gets merged, then it should be possible to test this out in the PR.
The other thing I would recommend is splitting this out into multiple files. I would probably go with main.yaml
(or maybe tests.yaml
?), integration-tests.yaml
, and publish.yaml
. It will keep them more discrete and readable. It will also eliminate the need for the various if:
blocks you've got in here.
good idea, i've created PR #1413 to bootstrap the CI
I'm not opposed to splitting it up however I'm a fan of the single workflow because i think it is visualized better in the GH runner console. Let's start with single workflow for now? |
Sounds good! Especially since you've got this already set up. And if you wanted to see an example of what it looks like split out, that's what I set up at my previous job in vulnbot, https://github.com/underdog-tech/vulnbot |
It looks like if you use https://github.com/marketplace/actions/python-poetry-action you won't need to worry about updating poetry every time. And you can even cache the installation! (Plus at least from my perspective, the author is much better known) |
.github/workflows/main.yaml
Outdated
role-session-name: GHA-${{ github.repository_owner }}-${{ github.event.repository.name }}-${{ github.run_id }} | ||
role-duration-seconds: ${{ env.AWS_ROLE_DURATION }} | ||
- name: run tests | ||
run: poetry run behave integration-tests/features --junit --junit-directory build/behave |
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.
ugh! i think something has changed in AWS which is causing some of these tests to fail now. It's now producing errors like WaitConditionHandle does not support any properties
. I'm guessing that it's related to aws-cloudformation issue #1097.
Circleci builds have been turned off because two systems cannot run integration tests against the same AWS account. I think we should comment out integration tests for now so that we at least have GH action testing in place for new PRs. The integration test fix might be more involved so I suggest we fix those in a follow on PR.
thanks for the tip. I didn't use python-poetry-action because it wasn't as well documented as the install-poetry-action. Either one seems fine to me since both have tons of stars in the marketplace. Lets just leave this optimization for a follow on PR? |
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.
Looks like a great start!
Move the Sceptre CI from using circleci to github actions. This also adds the usage of Github OIDC access to AWS to allow the execution of integration tests against an AWS account without providing github with user credentials.
depends on Sceptre/sceptre-aws#14