generated from MITLibraries/python-cli-template
-
Notifications
You must be signed in to change notification settings - Fork 0
In 1103 reconcile cli command #56
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
bf5b0f0
Refactor BaseWorkflow class and add CLI options
ehanson8 0fb1195
Add reconcile CLI command
ehanson8 e7822ef
Update Pipfile.lock
ehanson8 0c860b0
Updates based on discussion in #56
ehanson8 059a282
Updates based on further discussion in #56
ehanson8 dc9aa7e
More updates based on discussion in PR #56
ehanson8 80e135c
Correct variable names
ehanson8 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or 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
This file contains hidden or 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
Empty file.
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Our recent discussions around how the workflows will be initialized made me wonder how these values are set for workflow instances.
Recalling what was written in the DSC engineering plan:
Expanding the instructions in the Example Application Run, these are the steps I figured I'd need to do:
Prerequisite
Running
reconcilecommand.envfile at the root of the DSC app directory. Looking at the current definition of thereconcilecommand, it seems like it needs to know the following values for a given workflow:[s3_bucket, batch_path]. The.envfile should look like:DSC_S3_BUCKET="dsc"reconcilecommand in terminal:Running
depositcommandOption A: Running
depositvia the DSC CLI.envfile at the root of the DSC app directory.depositcommand in terminalOption B: Running
depositECS task using AWS CLISet up a JSON file with overrides for the ECS task (e.g.,
overrides.json){ "containerOverrides": [ { "name": "dsc-ecs-<env>", "command": [ "-w", "<workflow-name>", "-b", "<batch-id>", "-c", "<collection-handle>", "deposit" ], "environment": [ { "name": "DSC_S3_BUCKET", "value": "dsc" }, { "name": "DSC_SUBMISSION_SYSTEM", "value": "DSpace@MIT" }, { "name": "DSC_OUTPUT_QUEUE", "value": "<output-queue-name>" } ] } ] }Run
aws ecs run-taskCLI command in terminal (see CarbonMakefilefor example).Option C: Running
depositECS task using ??? DSC CLI command using theboto3library.Note: This is how I interpreted what is currently written in the Engineering Plan. 🤔 I believe the idea was from our recent work with the Alma SAP Invoices UI where we defined an
ECSClientto run ECS tasks.This work needs to be scoped out some more, but in terms of passing...workflow parameter values, but it should support accessing workflow parameter values set in a
.envvia theConfig.Takeaways
Environment variable names subject to change (curious what folks would find helpful)🤓
Change request: Include additional variables related to
reconcile,deposit, andfinalizeDSC CLI commands toREQUIRED_ENV_VARSandOPTIONAL_ENV_VARSindsc.Config.Request: Have a discussion as a team to scope out workflow implementation~
depositCLI command.ECSClientAlso, I think this started with having some questions about how the workflow attributes retrieve values.
workflow_nameandmetadata_mappingto be hardcoded in the class definition.submission_system,email_recipients, ands3_bucketshould be accessed as environment variables via theConfig. 🤔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.
As noted by @ehanson8 in Slack, the questions/comments above do not need to be addressed via this PR, but it has prompted a discussion about workflow implementation to be addressed in future PRs!
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.
We synced up on env vars and arrived at the following decision: