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: fix sign command options & show global options in subcommand help #4

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

WhistlingZephyr
Copy link
Contributor

Hello! It's me again. I was using this repo's CLI again and noticed that in v0.6.0's sign command was giving me this error:
image
And after realizing from the check of

verifyKeys(options, ['apiKey', 'apiSecret', 'addonId', 'addonVersion']);
my error only complained about every required option other than addonVersion which was explicitly defined inside the subcommand in

amo-upload/src/bin.ts

Lines 29 to 35 in 959bc26

program
.command('sign')
.description(
`Upload a new version for signing, check the status and download the signed file.
This command could be run multiple times to check the status, and the version will not be uploaded repeatedly.`,
)
.option('--addon-version <addonVersion>', 'the version to create or query')
I found that for the sign command any of the global options weren't being passed at all. I noticed the list subcommand already has a solution for that in

amo-upload/src/bin.ts

Lines 96 to 102 in 959bc26

.action(
wrapError(async (_, command) => {
const options = {
...loadFromEnv(),
...command.optsWithGlobals(),
};
verifyKeys(options, ['apiKey', 'apiSecret', 'addonId']);
so I copied it over to the sign command as well.

I also added the showGlobalOptions flag for commander.js mentioned in their readme and example that I found while looking up info on the initial error.

showGlobalOptions: show a section with the global options from the parent command(s)

I know v0.6.0 was pushed 10 months ago so I'm a bit late to the party, but would still appreciate having this CLI fix merged. Thanks!

@WhistlingZephyr
Copy link
Contributor Author

WhistlingZephyr commented Oct 17, 2024

Uhh, the CI is failing with

UsageError: This project is configured to use yarn because /home/runner/work/amo-upload/amo-upload/package.json has a "packageManager" field

I did notice the repo was mistakenly using yarn in .husky/pre-push:

Not sure where else it's using that though (couldn't find a packageManager field in the package.json file), let me update that part and see if it fixes.

@WhistlingZephyr
Copy link
Contributor Author

No luck, CI is still failing...

@WhistlingZephyr
Copy link
Contributor Author

WhistlingZephyr commented Oct 17, 2024

Could you try clearing caches from https://github.com/violentmonkey/amo-upload/actions/caches maybe? I think it could be pulling old cache where node_modules still used yarn. I can't find any more traces of yarn in the code.

@gera2ld
Copy link
Member

gera2ld commented Oct 18, 2024

Could you rebase to the lastest main branch? I've updated the github actions.

@WhistlingZephyr
Copy link
Contributor Author

WhistlingZephyr commented Oct 18, 2024

Done, CI works now.

@gera2ld gera2ld merged commit 6d5b810 into violentmonkey:main Oct 22, 2024
1 check 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.

2 participants