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

fix: Fixed manifest download cli to download output for selected step … #603

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

sakshie95
Copy link
Contributor

Fixes: #602

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

Manifest download cli does not download output manifests as it is intended to do for the provided step-id or the entire job if no step id is provided.

What was the solution? (How)

Added changes to download both input & output manifests (for step & dependencies if step-id is provided or for entire job if no step-id is provided) as the default behaviour. Also, added the option for downloading only input or output if customer wants to do that.

What is the impact of this change?

Manifest download clis will work as intended

How was this change tested?

Added integ tests, ran existing tests and tested the changes manually with the following steps:
hatch env prune
hatch build
pip install <wheel file> --force-reinstall
hatch run test
hatch run integ:test

Manual test download cli: deadline manifest download --job-id <job-id> --farm-id <farm-id> --queue-id <queue-id> <download location>

  • Have you run the unit tests? yes
  • Have you run the integration tests? yes
  • Have you made changes to the download or asset_sync modules? If so, then it is highly recommended
    that you ensure that the docker-based unit tests pass. No

Was this change documented?

  • Are relevant docstrings in the code base updated? yes
  • Has the README.md been updated? If you modified CLI arguments, for instance.

Does this PR introduce new dependencies?

This library is designed to be integrated into third-party applications that have bespoke and customized deployment environments. Adding dependencies will increase the chance of library version conflicts and incompatabilities. Please evaluate the addition of new dependencies. See the Dependencies section of DEVELOPMENT.md for more details.

  • This PR adds one or more new dependency Python packages. I acknowledge I have reviewed the considerations for adding dependencies in DEVELOPMENT.md.
  • This PR does not add any new dependencies.

Is this a breaking change?

N/A, because the manifest cli & api is in beta. Also default behaviour for the cli was always intended to be to download 'all' but it downloads only input right now.

Does this change impact security?

N/A


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

@sakshie95 sakshie95 requested review from a team as code owners February 14, 2025 20:19
@sakshie95 sakshie95 changed the title fix: Fixed manifet download cli to download output for selected step … fix: Fixed manifest download cli to download output for selected step … Feb 14, 2025
@sakshie95 sakshie95 force-pushed the mainline branch 2 times, most recently from 58031e3 to 6eddd16 Compare February 14, 2025 23:57
@sakshie95 sakshie95 force-pushed the mainline branch 4 times, most recently from 80a1ed4 to e2050fd Compare February 18, 2025 23:35
godobyte
godobyte previously approved these changes Feb 19, 2025
leongdl
leongdl previously approved these changes Feb 20, 2025
Copy link
Contributor

@leongdl leongdl left a comment

Choose a reason for hiding this comment

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

OK as long as we fix the next_token in a fast follow.

@sakshie95 sakshie95 dismissed stale reviews from leongdl and godobyte via 6b69024 February 20, 2025 21:19
Copy link
Contributor

@leongdl leongdl left a comment

Choose a reason for hiding this comment

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

Done - new diffs compared to last approval was help text only.

… or job by default. Added options to download only input or output too.

Signed-off-by: Sakshi <[email protected]>
@sakshie95 sakshie95 merged commit b2285fb into aws-deadline:mainline Feb 20, 2025
25 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.

Bug: Manifest download cli does not download all output manifests
5 participants