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

[Resolves #1401] Move CI to github actions #1412

Merged
merged 19 commits into from
Jan 15, 2024
Merged

Conversation

zaro0508
Copy link
Contributor

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

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
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
Copy link
Contributor Author

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.

Copy link
Member

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:

  1. 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.
  2. 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?

Copy link
Contributor Author

@zaro0508 zaro0508 Jan 15, 2024

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.

Copy link

dpulls bot commented Jan 14, 2024

🎉 All dependencies have been resolved !

.github/workflows/main.yaml Outdated Show resolved Hide resolved
Copy link
Member

@tarkatronic tarkatronic left a 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.

@zaro0508
Copy link
Contributor Author

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.

good idea, i've created PR #1413 to bootstrap the CI

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.

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?

@tarkatronic
Copy link
Member

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.

good idea, i've created PR #1413 to bootstrap the CI

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.

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

@tarkatronic
Copy link
Member

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)

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
Copy link
Contributor Author

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.

@zaro0508
Copy link
Contributor Author

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)

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?

@zaro0508 zaro0508 requested a review from tarkatronic January 15, 2024 17:37
Copy link
Member

@tarkatronic tarkatronic left a 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!

@zaro0508 zaro0508 merged commit e64c9f4 into Sceptre:master Jan 15, 2024
12 checks passed
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.

2 participants