-
Notifications
You must be signed in to change notification settings - Fork 76
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
@W-15803283@ - BREAKING CHANGE: Migrating from sfdx to sf #316
@W-15803283@ - BREAKING CHANGE: Migrating from sfdx to sf #316
Conversation
Thanks for the contribution! It looks like @BharathAdhikari is an internal user so signing the CLA is not required. However, we need to confirm this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, great work! I just have some comments.
@@ -362,12 +362,12 @@ OPTIONS | |||
this command invocation | |||
|
|||
EXAMPLES | |||
sfdx commerce:files:copy -y --copySourcePath "~/myexamplefilesdirectory" --filestocopy "file1.txt,file2.txt" | |||
sf commerce:files:copy -y --copySourcePath "~/myexamplefilesdirectory" --filestocopy "file1.txt,file2.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With sf commands, the convention is to not use colons (sf commerce files copy
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but as the sf command is taking care of this conversion haven't updated the commands as we might have to also update the structure at the file level.
const productsFileCsv: string = this.flags['products-file-csv'] as string; | ||
const username: string = this.org.getUsername(); | ||
|
||
const command = `sfdx shane:data:file:upload -f ${productsFileCsv} -u ${username} --json`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does sf shane data file upload
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is working as of now; but there is also an update from the SF team that they are releasing a new command to support this import functionality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the sf equivalent implementation.
@@ -136,7 +136,7 @@ export function appendCommonFlags(cmd: string, flags: OutputFlags<any>, logger: | |||
if (flags?.apiversion && cmd.startsWith('sf ')) { | |||
cmd = `${cmd} --api-version=${flags.apiversion as string}`; | |||
} else if (flags?.apiversion) { | |||
cmd = `${cmd} --apiversion=${flags.apiversion as string}`; | |||
cmd = `${cmd} --api-version=${flags.apiversion as string}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this change works just remove the whole if else block and just keep this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just kept it till we update the Shane command as it is still using sfdx
test/lib/utils/shell.test.ts
Outdated
// assert.ok(res.res.indexOf('result') >= 0); | ||
// }); | ||
// }); | ||
// describe('Shell Json Sfdx', () => { | ||
// it('should get api version in an object', async function () { | ||
// this.timeout(20000); | ||
// const res = shellJsonSfdx<{ apiVersion }>('sfdx force'); | ||
// const res = shellJsonSfdx<{ apiVersion }>('sf force'); | ||
// assert.ok(res.result.apiVersion); | ||
// }); | ||
// }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just delete this test, since it is commented out already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will remove it.
@tarcang as discussed, verified the e2e pipeline by adding 250(code-line) devhub credentials. Attaching the e2e pipeline run status for ref : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! Please review the comments.
@@ -12,101 +12,95 @@ | |||
|
|||
1. Install Salesforce CLI. You can install the CLI with either npm or with a downloadable installer for your specific operating system. See [Salesforce CLI Setup Guide](https://developer.salesforce.com/docs/atlas.en-us.sfdx_setup.meta/sfdx_setup/sfdx_setup_install_cli.htm) for more information. | |||
```bash | |||
npm install --global sfdx-cli | |||
npm install @salesforce/[email protected] --global |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't have to enter a version name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import functionality was pushed as a part of 2.43.7 itself and as the pipelines try to install 2.42.x. the new SF import command fails, Hence added with the specific version.
Will remove this before moving this to main/release.
|
||
```bash | ||
sfdx commerce:store:create -n <<STORE_NAME>> -o b2c -b <<BUYER_USER_EMAIL>> -u <<ORG_USERNAME>> -v <<DEVHUB_USERNAME>> --apiversion=<<API_VERSION>> | ||
sf commerce:store:create -n <<STORE_NAME>> -o b2c -b <<BUYER_USER_EMAIL>> -u <<ORG_USERNAME>> -v <<DEVHUB_USERNAME>> --apiversion=<<API_VERSION>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure that using sf with colons still work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed this from here: https://oclif.io/docs/topic_separator, But was not able to verify this locally.
What does this PR do?
Migrating from the SFDX CLI to SF
What issues does this PR fix or reference?
#, W-15803283
Functionality Before
Was able to create a scratch org and sample store on the org created.
Functionality After
we can create a scratch org and sample store on the org created.
How to Test/Testing Effort
Verified the scratch org creation locally after updating the commands
For store creation, there is an existing issue with the schema where we were unable to deploy the retrieved digital experience bundles due to a schema issue. Will verify this flow once the issue is fixed.
Verified the e2e changes by testing with 250(code line)