Skip to content

Conversation

@sotojn
Copy link

@sotojn sotojn commented Mar 11, 2025

This PR makes the following changes:

  • Adds dry-run parameter and output parameter to the cli
    • dry-run will allow the user to run the command gather metadata on what was filtered from the github api
      • dry-run will display only the release name in the case that -q quiet is also enabled
    • output accepts either json or text(default) for the user to choose what format the dry-run info will printed in stdout

@sotojn sotojn self-assigned this Mar 11, 2025
@sotojn sotojn requested review from busma13 and godber March 13, 2025 15:20
Copy link

@busma13 busma13 left a comment

Choose a reason for hiding this comment

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

➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -d terascope standard-assets
Release: terascope/standard-assets@v1.3.3

The following files would have been downloaded:

- standard-v1.3.3-node-18-bundle.zip
- standard-v1.3.3-node-20-bundle.zip
- standard-v1.3.3-node-22-bundle.zip
- 
➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -dq terascope standard-assets
terascope/standard-assets@v1.3.3

➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -dq -o json terascope standard-assets
"terascope/standard-assets@v1.3.3"

➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -d -o json terascope standard-assets
{
  "release": "terascope/standard-assets@v1.3.3",
  "assetFileNames": [
    "standard-v1.3.3-node-18-bundle.zip",
    "standard-v1.3.3-node-20-bundle.zip",
    "standard-v1.3.3-node-22-bundle.zip"
  ]
}

I feel like dryrun, quiet, with json output is pretty useless. I'm not sure what change to suggest.

The readme needs to be updated to show the new options. That can wait until later if we want to get this in.

@sotojn
Copy link
Author

sotojn commented Mar 13, 2025

➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -d terascope standard-assets
Release: terascope/standard-assets@v1.3.3

The following files would have been downloaded:

- standard-v1.3.3-node-18-bundle.zip
- standard-v1.3.3-node-20-bundle.zip
- standard-v1.3.3-node-22-bundle.zip
- 
➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -dq terascope standard-assets
terascope/standard-assets@v1.3.3

➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -dq -o json terascope standard-assets
"terascope/standard-assets@v1.3.3"

➜  ~/WORKSPACE/fetch-github-release git:(b6bac64) ./bin/fetch-github-release -d -o json terascope standard-assets
{
  "release": "terascope/standard-assets@v1.3.3",
  "assetFileNames": [
    "standard-v1.3.3-node-18-bundle.zip",
    "standard-v1.3.3-node-20-bundle.zip",
    "standard-v1.3.3-node-22-bundle.zip"
  ]
}

I feel like dryrun, quiet, with json output is pretty useless. I'm not sure what change to suggest.

The readme needs to be updated to show the new options. That can wait until later if we want to get this in.

I agree. The more I think about it, the more I feel like it's a poor and awkward implementation on how dry-run and quiet interact. Maybe we just have quiet uphold it's original implementation and hide all logs even if dry-run is enabled. Open to other considerations

@godber
Copy link
Member

godber commented Mar 13, 2025

I feel like dryrun, quiet, with json output is pretty useless. I'm not sure what change to suggest.

if the user supplies -q and -o json, it should just give the standard JSON output. -q is redundant in that case. -q is telling it to be less verbose basically. json is already not verbose.

@godber godber merged commit df7e537 into master Mar 13, 2025
4 checks passed
@godber godber deleted the dry-run-feat branch March 13, 2025 23:57
@godber
Copy link
Member

godber commented Mar 13, 2025

We can come back and resolve the -q thing.

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