-
Notifications
You must be signed in to change notification settings - Fork 112
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
.github: add action to run acceptance tests #380
Conversation
6395d6e
to
ec4a43a
Compare
PACKER_ACC=1 go test \ | ||
-timeout 3h \ | ||
-count 1 \ | ||
-run "${{ github.event.inputs.run_pattern }}" \ | ||
./builder/ebs |
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.
So a few suggestions here. I would expect the acceptance test to run all test, with the default run pattern to be "TestAcc" .
PACKER_ACC=1 go test \ | |
-timeout 3h \ | |
-count 1 \ | |
-run "${{ github.event.inputs.run_pattern }}" \ | |
./builder/ebs | |
PACKER_ACC=1 go test \ | |
-timeout 3h \ | |
-count 1 \ | |
-run "${{ github.event.inputs.run_pattern }}" \ | |
./... \ | |
-v |
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.
While I tend to agree, the rationale to separate the actions by builder was so packages could be tested in parallel (although I presume we could do this with an option to go test
maybe), and so they can have different timeout/token validity periods.
I can change it however, can't hurt to simplify for now and revisit later if the need to split between components arises.
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.
That's valid. If you take that approach take care to ensure that there are no tests that overlap with each other or that are sharing the same instance names that would cause any potential conflicts by running in parallel.
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.
True! If we see conflicts arising here we can always rename the instances in the problematic tests too, but we can take a look before to make sure it does work as intended before enabling other builders.
required: false | ||
run_pattern: | ||
description: "The pattern to run tests on" | ||
default: "*" |
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.
Using "*"
is an invalid regexp expression for go test
default: "*" | |
default: "TestAcc" |
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.
Good call, the pattern was borked indeed, I initially shot for .*
, but TestAcc
does the trick better.
|
||
name: "AWS Acceptance Tests" | ||
|
||
on: |
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.
Do you plan to add a scheduled cron after running your initial tests?
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.
Yes, for now while we're iterating on it I figured that would be enough, but later on I will indeed add a schedule, we can discuss how often would be relevant, but I'd go for once a week for starters?
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.
Whatever you think is best. I like nightly for active plugins.
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 don't mind nightly, but I fear this may become costly to run those every day, while once a week would strike a reasonable balance between costs and safety. We can augment/reduce the frequency if we think it runs too often/not often enough.
permissions: | ||
id-token: write | ||
contents: read | ||
name: AWS EBS Acceptance tests |
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.
Are you limiting to EBS just for the initial testing?
You may already know this but for the SSM tests we will need to install the aws cli and the session manager plugin. But calling out because the the aws-actions doesn't install it.
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.
Oh good call for the aws
cli and plugin, I completely forgot to install them. I'll add this to the workflow
make dev | ||
- uses: aws-actions/configure-aws-credentials@e1e17a757e536f70e52b5a12b2e8d1d1c60e04ef # v2 | ||
with: | ||
aws-access-key-id: "${{ env.AWS_ACC_TEST_KEY_ID }}" |
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.
Just out of my curiosity, the acceptance service account is used to authenticate and it the assumes another role for testing. Is that correct?
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.
That's the idea yes!
Since right now we don't have any way to run acceptance tests on the AWS plugin, we add a workflow that can manually be triggered.
ec4a43a
to
467eeb3
Compare
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
Since right now we don't have any way to run acceptance tests on the AWS plugin, we add a workflow that can manually be triggered.