Skip to content

Conversation

@ethanholz
Copy link
Contributor

@ethanholz ethanholz commented Dec 5, 2024

Adds numerous changes for making this workflow generic. Some docs are missing and I am missing tests for the new cloud deployment classes since I will utilize testing from the PyGithub changes in #42 to better construct mocking for these cases. For usage of this library see, https://github.com/omsf-eco-infra/start-aws-gha-runner and https://github.com/omsf-eco-infra/stop-aws-gha-runner.

@ethanholz
Copy link
Contributor Author

Current summary, generated by Copilot

This pull request includes significant changes to the cloud deployment architecture, introducing new classes and refactoring existing ones for better separation of concerns and clarity. Additionally, helper functions and classes have been added to manage environment variables and workflow commands.

Refactoring and New Classes:

  • src/gha_runner/clouddeployment.py: Refactored the CloudDeployment class into two new abstract base classes, CreateCloudInstance and StopCloudInstance, to separate the responsibilities of starting and stopping cloud instances. Introduced DeployInstance and TeardownInstance classes to handle the deployment and teardown processes respectively. [1] [2]

Helper Functions and Classes:

  • src/gha_runner/helper/helper.py: Added helper functions output, warning, and error to handle workflow commands and output messages.
  • src/gha_runner/helper/input.py: Introduced check_required function to validate required environment variables. Added EnvVarBuilder class to construct a dictionary of parameters from environment variables with support for JSON parsing and type conversion.
  • src/gha_runner/helper/workflow_cmds.py: Added functions output, warning, and error to manage GitHub Actions workflow commands.

@ethanholz ethanholz self-assigned this Dec 5, 2024
@ethanholz ethanholz force-pushed the refactor/convert-to-library branch from 356a977 to c3672be Compare December 9, 2024 22:16
@ethanholz ethanholz force-pushed the refactor/convert-to-library branch from c3672be to 77d5cd0 Compare December 9, 2024 22:46
@codecov
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 99.14530% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.65%. Comparing base (9a1a8ce) to head (eeb3888).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/gha_runner/clouddeployment.py 98.63% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
+ Coverage   95.20%   97.65%   +2.44%     
==========================================
  Files           3        4       +1     
  Lines         334      256      -78     
==========================================
- Hits          318      250      -68     
+ Misses         16        6      -10     
Flag Coverage Δ
unittests 97.65% <99.14%> (+2.44%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ethanholz ethanholz marked this pull request as ready for review December 11, 2024 23:11
@ethanholz
Copy link
Contributor Author

Notably, this PR is missing significant docs changes as I think those will be handled done separately as part of the larger migration. Additionally, this does not include changes for uploading to PyPI at this time and will happen following the merging of this PR. Let me know if you have an comments or concerns!

Copy link
Member

@dwhswenson dwhswenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. Inline comments have some questions and docstring fixes. Beyond the inline comments, two other points:

  • I'd prefer that the test structure better match the src structure. So, for example, since we have src/helpers/input.py, move tests/test_input.py to tests/helpers/test_input.py (makes a big difference in large projects). Same with workflow_cmds.

  • Question about design: why pass both cloud_params and provider_type to the DeployInstance? Does the DeployInstance need direct access to cloud_params? It seems like it would be better separation to just pass the provider instance to the DeployInstance and generate the provider as part of a separate step?

@ethanholz
Copy link
Contributor Author

  • Question about design: why pass both cloud_params and provider_type to the DeployInstance? Does the DeployInstance need direct access to cloud_params? It seems like it would be better separation to just pass the provider instance to the DeployInstance and generate the provider as part of a separate step?

The answer is yes. DeployInstance updates the cloud parameters with information from GitHub (specifically the runner tokens and the release platform). This makes it so that you have a very simplified pattern for implementation for all downstream users.

Initially, I had done it that way and that was reflected in the initial implementation of this project. My largest goal here when consolidating these pieces was to make it such that building a new provider requires two steps:

  1. Parse your parameters needed to start or stop into a single dict
  2. Implement the CreateCloudInstance class to consume that dict with the updated GitHub pieces.

Furthermore, I wanted to make it so that the end-developer has a simple API surface to use so that the main function is simple and can be reused across providers. This is embodied in the start-aws-gha-runner's main function as most of that function can be reused to use a new and custom provider.

@ethanholz ethanholz merged commit a67506e into omsf:main Jan 16, 2025
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