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 tarballs based commands not considering config from package.json #799

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/commands/pack/tarballs.ts
Expand Up @@ -11,20 +11,16 @@ This can be used to create oclif CLIs that use the system node or that come prel

static flags = {
root: Flags.string({char: 'r', description: 'path to oclif CLI root', default: '.', required: true}),
targets: Flags.string({char: 't',
description: 'comma-separated targets to pack (e.g.: linux-arm,win32-x64)',
default: Tarballs.TARGETS.join(','),
}),
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

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.

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

xz: Flags.boolean({description: 'also build xz', allowNo: true}),
tarball: Flags.string({char: 'l', description: 'optionally specify a path to a tarball already generated by NPM', required: false})
}

async run(): Promise<void> {
const prevCwd = qq.cwd()
if (process.platform === 'win32') throw new Error('pack does not function on windows')
const {flags} = await this.parse(PackTarballs)
const targets = flags.targets.split(',')
const buildConfig = await Tarballs.buildConfig(flags.root, {xz: flags.xz, targets: targets})
const buildConfig = await Tarballs.buildConfig(flags.root, {xz: flags.xz, targets: flags?.targets?.split(',')})
if (buildConfig.targets.length === 0) {
throw new Error('Please specify one or more valid targets.')
}
Expand Down
11 changes: 3 additions & 8 deletions src/commands/promote.ts
Expand Up @@ -15,24 +15,19 @@ export default class Promote extends Command {
version: Flags.string({description: 'semantic version of the CLI to promote', required: true}),
sha: Flags.string({description: '7-digit short git commit SHA of the CLI to promote', required: true}),
channel: Flags.string({description: 'which channel to promote to', required: true, default: 'stable'}),
targets: Flags.string({
char: 't',
description: 'comma-separated targets to promote (e.g.: linux-arm,win32-x64)',
default: Tarballs.TARGETS.join(','),
}),
targets: Flags.string({char: 't', description: 'comma-separated targets to promote (e.g.: linux-arm,win32-x64)'}),
deb: Flags.boolean({char: 'd', description: 'promote debian artifacts'}),
macos: Flags.boolean({char: 'm', description: 'promote macOS pkg'}),
win: Flags.boolean({char: 'w', description: 'promote Windows exe'}),
'max-age': Flags.string({char: 'a', description: 'cache control max-age in seconds', default: '86400'}),
xz: Flags.boolean({description: 'also upload xz', allowNo: true, default: true}),
xz: Flags.boolean({description: 'also upload xz', allowNo: true}),
indexes: Flags.boolean({description: 'append the promoted urls into the index files'}),
}

async run(): Promise<void> {
const {flags} = await this.parse(Promote)
const maxAge = `max-age=${flags['max-age']}`
const targets = flags.targets.split(',')
const buildConfig = await Tarballs.buildConfig(flags.root, {targets})
const buildConfig = await Tarballs.buildConfig(flags.root, {targets: flags?.targets?.split(',')})
const {s3Config, config} = buildConfig
const indexDefaults = {
version: flags.version,
Expand Down
13 changes: 4 additions & 9 deletions src/commands/upload/tarballs.ts
Expand Up @@ -15,19 +15,14 @@ export default class UploadTarballs extends Command {

static flags = {
root: Flags.string({char: 'r', description: 'path to oclif CLI root', default: '.', required: true}),
targets: Flags.string({
char: 't',
description: 'comma-separated targets to upload (e.g.: linux-arm,win32-x64)',
default: Tarballs.TARGETS.join(','),
}),
xz: Flags.boolean({description: 'also upload xz', allowNo: true, default: true}),
targets: Flags.string({char: 't', description: 'comma-separated targets to upload (e.g.: linux-arm,win32-x64)'}),
xz: Flags.boolean({description: 'also upload xz', allowNo: true}),
}

async run(): Promise<void> {
const {flags} = await this.parse(UploadTarballs)
if (process.platform === 'win32') throw new Error('upload does not function on windows')
const targets = flags.targets.split(',')
const buildConfig = await Tarballs.buildConfig(flags.root, {targets, xz: flags.xz})
const buildConfig = await Tarballs.buildConfig(flags.root, {xz: flags.xz, targets: flags?.targets?.split(',')})
const {s3Config, dist, config, xz} = buildConfig

// fail early if targets are not built
Expand Down Expand Up @@ -77,7 +72,7 @@ export default class UploadTarballs extends Command {
await aws.s3.uploadFile(dist(manifest), {...ManifestS3Options, Key: cloudKey})
}

if (targets.length > 0) log('uploading targets')
if (buildConfig.targets.length > 0) log('uploading targets')
// eslint-disable-next-line no-await-in-loop
for (const target of buildConfig.targets) await uploadTarball(target)
log(`done uploading tarballs & manifests for v${config.version}-${buildConfig.gitSha}`)
Expand Down
2 changes: 1 addition & 1 deletion src/tarballs/config.ts
Expand Up @@ -86,7 +86,7 @@ export async function buildConfig(root: string, options: {xz?: boolean; targets?
config,
tmp,
updateConfig,
xz: typeof options.xz === 'boolean' ? options.xz : Boolean(updateConfig.s3.xz),
xz: options?.xz ?? updateConfig?.s3?.xz ?? true,
dist: (...args: string[]) => path.join(config.root, 'dist', ...args),
s3Config: updateConfig.s3,
nodeVersion,
Expand Down