-
Notifications
You must be signed in to change notification settings - Fork 13
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
feat!: rework package to facilitate worker agent integ tests #6
Conversation
61ced4b
to
a8f1bcf
Compare
a8f1bcf
to
03640d8
Compare
03640d8
to
7d5b8e6
Compare
7d5b8e6
to
eeab60c
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.
Just want to comment on some thoughts/ideas that I had while modifying these fixtures.
Currently, these fixtures are highly opinionated towards Worker agent integration tests. e.g. the deadline_resources
fixture always deploys a Farm + Queue + Fleet setup. While we could reuse this fixture for JobAttachments integ tests, it doesn't feel right to be deploying a Fleet + QFA when we don't need it for those tests.
Similarly, to maintain backwards compatibility with JA integ tests, I've kept the existing deploy_job_attachment_resources
fixture and the related JobAttachmentManager
class. These are also specifically for Job Attachments.
So.. that got me wondering if we actually want all these fixtures to be defined in this package. Currently, basically all fixtures aside from deploy_job_attachment_resources
will only be used by Worker tests, while deploy_job_attachment_resources
will only be used by Job Attachments. Maybe it makes more sense to move these fixtures into their respective packages instead, because they are so specific.
The fixtures that I think do make sense to keep here need to be common/generic enough that multiple packages can reuse them in their own fixture implementations.
e.g. when we get around to adding deadline-cloud
integration tests, it probably makes sense to provide a default Farm
and Queue
fixture so we can test out job submission.. and building on top of that, the worker
fixture might also make sense to keep in here for when we eventually do full E2E tests in the future (DCC -> deadline-cloud CLI -> service -> Worker agent),
Thoughts?
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.
For now, we can keep it as is since it's pretty early in the game to be making long-term organizational decisions like this, but just wanted to mention 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.
Wow. This is a lot of code, and very well done. You've basically created our own CDK-lite
It definitely needs more commenting, and a DEVELOPER.md to help navigate it. We can do those as fast-follows.
@@ -1,30 +1,101 @@ | |||
## Deadline Test Scaffolding | |||
# Deadline Cloud Test Fixtures |
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.
For folk to be able to use this it would be ideal to have the minimal IAM Policy that one needs to use the fixtures (i.e. deploy the Cfn stacks) in the README.
Figuring that out can be some serious whack-a-mole, so let's not hold up this PR to gather that, but we will need to figure out that minimal policy when integrating this in to our integration test runners.
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.
Sounds good, I've got a backlog ticket to address this
self.worker_role = Role( | ||
self, | ||
"WorkerRole", | ||
role_name="DeadlineScaffoldingWorkerRole", |
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.
A hard-coded name like this will mean that an account can only ever have a single instance of the integration test scaffold running/deployed at a time. What do you think about adding a random or time element to the name to make it possible to have multiple going at once?
Ditto for other roles.
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 a good point. In it's current state, this infrastructure is only designed to be deployed once across all tests (the CfnStack.deploy
method will try to update an existing stack name, and our fixtures are session
scoped). I think for now, this is sufficient since it should cover most of our test cases for the Worker agent.
For the test cases where we need a misconfigured AWS resource like an IAM role, which would be the value of having multiple stack deployments, we can either define another stack with the misconfigured resources, or change this one to be configurable.
I've posted a comment that's somewhat related to this: #6 (comment). I think that when we do need the misconfigured AWS resources stack, it should belong in the Worker agent repo, since other repos most likely don't care about a Worker that doesn't work
"Effect": "Allow", | ||
"Action": [ | ||
"deadline:CreateWorker", | ||
"deadline:GetWorkerIamCredentials", |
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.
GetWorkerIamCredentials
does not exist. This can be removed.
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.
Thanks, removed
What was the problem/requirement? (What/Why)
While working on writing integration tests for deadline-cloud-worker-agent, I realized there could be some improvements to our test library to make it easier to write tests
What was the solution? (How)
add-copyright-headers.sh
script and made the copyright header test run by defaultWhat is the impact of this change?
It's a little easier to use this package for integration tests (specifically for Worker agent)
How was this change tested?
Was this change documented?
Yes, added to README.md and an example configuration file
Is this a breaking change?
Yes
Changes
cf_templates/
: Replaced by code incloudformation/
cloudformation/
: Models CloudFormation templates and handles deploymentcfn.py
: Core model for CloudFormation resourcesjob_attachments_bootstrap_stack.py
: Replacement for old CFN template undercf_templates/
worker_bootstrap_stack.py
: Stack deploying bootstrap resources for Worker agent deploymentdeadline/
: Models Deadline Cloud resources. Currently, this includes:client.py
: The existing shim layer we had for the boto client to handle API changesstubs.py
: The existing stub for the boto client that we use in some unit testsresources.py
: Models Deadline Cloud resources (e.g. Farm, Queue, Fleet, Job, etc.)worker.py
: Handles deploying a Worker agent.--no-install-service
option to its installerfixtures.py
: The number of fixtures has been reduced since the functionality has moved into objects that are returned by the fixtures. Currently, we have:deadline_client
: Our shim layer boto client that is configurable withDEADLINE_ENDPOINT
env varcodeartifact
: Stores info about the CodeArtifact repo to pull Python deps from based on env vars. Currently only used for Worker.service_model_s3_object
: Allows specifying the model file to use which is stored in S3 viaDEADLINE_SERVICE_MODEL_S3_URI
. Currently only used for Worker.bootstrap_resources
: Deploys the CloudFormation stack for bootstrapping Worker resources. Only used for Worker.deadline_resources
: Deploys an opinionated set of Deadline Cloud resources geared towards the Worker.worker
: Deploys a Worker.deploy_job_attachment_resources
: The existing fixture used by Job Attachments integ testsjob_attachment_manager
: Changed to use newdeadline/
andcloudformation/
code.models.py
: Contains common dataclasses. Open to suggestions for the module name.pytest_hooks.py
: Defines pytest hooks to enhance live logging for integ testsdeadline_manager.py
: Replaced by code indeadline/
constants.py
:fixtures.py
util.py
: Changed to contain utility functions used across the packagetest/unit
: Updated/added unit tests. Addedmoto
as a test dependency to make testing easier.