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

refactor: Refactoring EC2InstanceWorker to split out PosixInstanceWorker and WindowsInstanceWorker #125

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

ttblanchard
Copy link
Contributor

@ttblanchard ttblanchard commented Jul 3, 2024

What was the problem/requirement? (What/Why)

When we added the ability to support Windows workers we didn't use proper abstractions and ended up with a whole lot of code that does checks like this:

        if self.configuration.operating_system.name == "AL2023":
            userdata = self.linux_userdata(s3_files)
        else:
            userdata = self.windows_userdata(s3_files)

This is hard to maintain, read, and extend.

There were also some minor bugs discovered in the windows worker fixture implementation that were fixed as part of this change.

What was the solution? (How)

Make EC2InstanceWorker an abstract class with two concrete classes PosixInstanceWorker and WindowsInstanceWorker which inherit from the EC2InstanceWorker allowing us to remove all the branching code.

What is the impact of this change?

More readable and maintainable test fixtures.

How was this change tested?

I tested with a custom AMI with the worker agent installed - submitting jobs using both windows and linux workers.

Was this change documented?

No

Is this a breaking change?

Yes - libraries that use this package will need to update their tests to explicitly use either PosixInstanceWorker or WindowsInstanceWorker.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

moorec-aws
moorec-aws previously approved these changes Jul 4, 2024
@jusiskin
Copy link
Contributor

jusiskin commented Jul 4, 2024

This is reported as a breaking change in the PR description but does not indicate so in the PR/commit title according to conventional commits.

I recommend using the title and footer style. This has the benefits of both:

  1. identifying breaking changes when using git log or looking at commit history in GitHub
  2. providing a clearer description of the breaking change in the footer which can be used to automatically populate the changelog on release

src/deadline_test_fixtures/deadline/worker.py Show resolved Hide resolved
src/deadline_test_fixtures/deadline/worker.py Outdated Show resolved Hide resolved
src/deadline_test_fixtures/deadline/worker.py Outdated Show resolved Hide resolved
src/deadline_test_fixtures/deadline/worker.py Show resolved Hide resolved
src/deadline_test_fixtures/deadline/worker.py Outdated Show resolved Hide resolved
src/deadline_test_fixtures/deadline/worker.py Outdated Show resolved Hide resolved
test/unit/deadline/test_worker.py Outdated Show resolved Hide resolved
test/unit/deadline/test_worker.py Outdated Show resolved Hide resolved
…rker

and WindowsInstanceWorker

BREAKING CHANGE: EC2InstanceWorker can no longer be instanciated, use
WindowsInstanceWorker or PosixInstanceWorker instead.

Signed-off-by: Trevor Blanchard <55503092+ttblanchard@users.noreply.github.com>
@ttblanchard ttblanchard dismissed jusiskin’s stale review July 15, 2024 15:55

Changes have been addressed and others have followed up with reviews of their own.

@ttblanchard ttblanchard merged commit 5705df4 into aws-deadline:mainline Jul 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.

4 participants