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

Wr/manifest generate #168

Merged
merged 21 commits into from
Aug 12, 2021
Merged

Wr/manifest generate #168

merged 21 commits into from
Aug 12, 2021

Conversation

WillieRuemmele
Copy link
Contributor

@WillieRuemmele WillieRuemmele commented Aug 3, 2021

What does this PR do?

adds the sfdx force:source:manifest:create command.

I think that the NUT coverage should be enough, the command is so small that unit tests seemed extra when the ComponentSetBuilder is already covered

What issues does this PR fix or reference?

@W-9608078@

src/commands/force/source/manifest/create.ts Outdated Show resolved Hide resolved
src/commands/force/source/manifest/create.ts Outdated Show resolved Hide resolved
src/commands/force/source/manifest/create.ts Outdated Show resolved Hide resolved
description: messages.getMessage('flags.outputdir'),
}),
};
public requiredFlags = ['metadata', 'sourcepath'];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a better name for this would be xorFlags, meaning 1 and only 1 of them must be set when running the command. I think this should really move into oclif at some point too.

@@ -21,6 +21,7 @@ export type ProgressBar = {

export abstract class SourceCommand extends SfdxCommand {
public static readonly DEFAULT_SRC_WAIT_MINUTES = 33;
public requiredFlags: string[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be protected not public.

@@ -30,6 +31,15 @@ export abstract class SourceCommand extends SfdxCommand {
return getBoolean(this.flags, 'json', false);
}

protected validateFlags(flags: string[]): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to pass in the flags. SourceCommand has them as this.flags

},
setupCommands: [
'sfdx force:org:create -f config/project-scratch-def.json --setdefaultusername --wait 10 --durationdays 1',
'sfdx force:source:deploy -p force-app',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the test need to create an org and deploy as part of setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that way it would validate that the generated manifest was would work with a deploy. SDR should be verifying that getPackageXml() returns a valid package... but I thought it would be pretty easy to do here too

' <members>TestSampleDataController</members>\n' +
' <name>ApexClass</name>\n' +
' </types>\n' +
' <version>52.0</version>\n' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you override the API version this will fail as soon as 52.0 is not current.

WillieRuemmele and others added 4 commits August 5, 2021 17:13
* fix: bump SDR to 4.0.2

* chore: disable sfdx NUTs

* chore: disable sfdx executible in generateNut
"metadata",
"outputdir",
"sourcepath",
"targetusername"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shetzel to use SfdxCommand's standardized --apiversion flag I had to set supportsUsername=true which also includes the --targetusername... Do you think it would be better to duplicate the apiversion flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless the command will use that flag (-u) I would not set supportsUsername=true. Just duplicate the apiversion flag. There might be a way to reference it without duplicating.

Copy link
Contributor

@shetzel shetzel left a comment

Choose a reason for hiding this comment

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

Nice!

@shetzel shetzel merged commit cdad337 into main Aug 12, 2021
@shetzel shetzel deleted the wr/manifestGenerate branch August 12, 2021 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants