-
Notifications
You must be signed in to change notification settings - Fork 0
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: execute Python file scripts instead of in-line scripts #23
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BryanttV
approved these changes
Sep 30, 2024
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 for the PR @mariajgrimaldi! Just a small comment
dcoa
reviewed
Oct 1, 2024
dcoa
approved these changes
Oct 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR is a continuation of a proposal I made here about making scripts more maintainable in the long term. I mentioned I had to figure out a good way of calling scripts within our workflow. Here's what I came up with:
Python scripts hosted in
.github/workflows/scripts
The additional functionalities with complex logic for the workflow should be implemented in a Python script hosted in a subdirectory within
.github/workflows/scripts
with an explicit name expressing its purpose. In this case, the script gets tutor configuration values, so the Python script is calledget_tutor_config.py
. Each script should be well-documented and modular, have a single responsibility, and be able to be executed locally for easier debugging.Call scripts from reusable workflow
Scripts should be executed from a new step in the workflow that passes the necessary arguments and handles the scripts' output.
For this reusable workflow to use files from its repo, we must implement an explicit step to check out the code since, by definition, reusable workflows check out code from the caller workflow repository: https://docs.github.com/en/actions/sharing-automations/reusing-workflows#overview
This explains why we added an extra step here, and why we're now checking out code in subdirectories, so there's no overriding.
I consider this approach more maintainable since we can now easily lint, test, and debug scripts by removing the overhead of having hard-coded logic within the workflow. The scripts hosted in
.github/workflows/scripts
are meant to handle all complex logic to avoid returning to maintaining our previous approach.How to test
ednx-strains/.github/build.yml
workflow execution, go to Actions > Build Open edX strain, fill in the necessary configuration, please use the workflow version fromMJG/scripts-folder
. For the strain to build, I suggest using theMJG/scripts-folder
strain branch to avoid overriding the current image on dockerhub. In this case, you don't need to wait for the workflow to finish to know it works, just check Get Tutor Configurations from config.yml and set them as an environment variable the stepGet Tutor Configurations from config.yml and set them as an environment variable
is successful.For a successful build, use MJG/scripts-success.
For a build with missing configurations, use MJG/scripts-fail.
For a build with a different registry (optional configuration) use MJG/support-diff-registries. To successfully push the image to Amazon ECR, you'll need to use the workflow from
MJG/support-diff-registries
as well.This implementation is not set in stone, so feedback is very welcome.