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.

Sign up for GitHub

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

Jump to bottom

Fix tarballs based commands not considering config from package.json #799

Merged
merged 6 commits into from Feb 17, 2022
Merged

Fix tarballs based commands not considering config from package.json #799

merged 6 commits into from Feb 17, 2022

Conversation

rannn505
Copy link
Contributor

  • Remove flags defaults from those commands
  • Let Tarballs.buildConfig to determine final config
  • Fix duplicate -t char for targets and tarball flags in oclif pack tarballs

@rannn505
Copy link
Contributor Author

@jdxcode not sure why this check fails 🤔
image

anyway without this fix I have to add --no-xz and --targets to pack, upload, promote commands

@rannn505
Copy link
Contributor Author

@mattgraham @chadian @RodEsp @oclif-bot anyone?

@rannn505
Copy link
Contributor Author

@mdonnalley @pimterry @peternhale please help 🙏🏽

@rannn505 rannn505 mentioned this pull request Feb 10, 2022
Closed
mdonnalley
mdonnalley requested changes Feb 16, 2022
View reviewed changes
src/commands/pack/tarballs.ts
}),
xz: Flags.boolean({description: 'also build xz', allowNo: true, default: true}),
tarball: Flags.string({ char: 't', description: 'optionally specify a path to a tarball already generated by NPM', required: false })
targets: Flags.string({char: 't', description: 'comma-separated targets to pack (e.g.: linux-arm,win32-x64)'}),
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to not remove the default here since that would be a breaking change

It might be a better solution to change this line to prefer updateConfig.node.targets over options.targets. However, that's not a great solution either since user's would never be able to override what's the configured targets

@peternhale @RodEsp Do you have any opinions on this? It looks like the default for the targets flag was added soon after the oclif-dev commands were migrated into oclif: #458

Copy link
Contributor Author

@rannn505 rannn505 Feb 16, 2022 •
edited

Choose a reason for hiding this comment

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

@mdonnalley Why is this a ״breaking change״?
Correct me if i'm wrong but it seems like if flags?.targets? is undefined the following line takes TARGETS array as a fallback (if not defined on package.json) which is exactly the same behaviour.

oclif/src/tarballs/config.ts

Line 70 in 71c3690

const targets = compact(options.targets || updateConfig.node.targets || TARGETS)

Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be the correct; command flag then config then default.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rannn505 you're right, I hadn't considered that

mdonnalley
mdonnalley previously approved these changes Feb 16, 2022
View reviewed changes
@rannn505
Copy link
Contributor Author

@mdonnalley I updated this branch from main and resolved all conflicts .. please re-approve this PR and lets handle the failing CI phase so we can go ahead and merge it .. I see more contributors complaining about that 🙏🏽 Thanks.
#799 (comment)

Regardless of the scope of this PR, I'm willing to get more and more involved and help this project grow if you want me to do so

@mdonnalley mdonnalley changed the base branch from main to mdonnalley/799 February 17, 2022 16:11
@mdonnalley mdonnalley merged commit 39f5372 into oclif:mdonnalley/799 Feb 17, 2022
@mdonnalley mdonnalley mentioned this pull request Feb 17, 2022
Merged
mdonnalley added a commit that referenced this pull request Feb 17, 2022
@mdonnalley @rannn505
Fix tarballs based commands not considering config from package.json (#…
c26946d 
…799) (#830)

* fix: remove flags defaults and let Tarballs.buildConfig to determine final config

* fix: applying fix on promote command

* fix: set xz true by default

* Update tarballs.ts

Co-authored-by: Ran Cohen <rannn505@outlook.com>
@rannn505
Copy link
Contributor Author

@mdonnalley , I sent you an email, hope you received it, and I'm happy to help if you let me. Thanks in advance.

gaelmotte pushed a commit to gaelmotte/oclif that referenced this pull request Oct 24, 2022
@mdonnalley @rannn505
Fix tarballs based commands not considering config from package.json (o…
1aa5bfe 
…clif#799) (oclif#830)

* fix: remove flags defaults and let Tarballs.buildConfig to determine final config

* fix: applying fix on promote command

* fix: set xz true by default

* Update tarballs.ts

Co-authored-by: Ran Cohen <rannn505@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdonnalley mdonnalley mdonnalley approved these changes

@peternhale peternhale Awaiting requested review from peternhale

Assignees
No one assigned
Labels
cla:signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rannn505 @mdonnalley @peternhale