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: correct apiVersion and sourceApiVersion values before transfer and send events #791

Merged
merged 7 commits into from
Dec 9, 2022

Conversation

shetzel
Copy link
Contributor

@shetzel shetzel commented Dec 6, 2022

What does this PR do?

Corrects the code that determines the apiVersion and sourceApiVersion before sending a deploy or retrieve. Also emits an event with this data just before sending so that consumers can act upon it (e.g., display useful output).

The proper priority for determining the API version of the connection (items higher on the list take precedence) is:

  1. apiversion command flag
  2. SFDX_API_VERSION env var
  3. local config var
  4. global config var
  5. highest version supported by the target org

The proper priority for determining the manifest version (items higher on the list take precedence) is:

  1. <version> in package.xml
  2. sourceApiVersion in sfdx-project.json
  3. apiversion command flag
  4. SFDX_API_VERSION env var
  5. local config var
  6. global config var
  7. highest version supported by the target org

What issues does this PR fix or reference?

@W-12146860@

@shetzel shetzel requested review from a team as code owners December 6, 2022 19:11
@shetzel shetzel requested a review from randi274 December 6, 2022 19:11
Copy link
Member

@WillieRuemmele WillieRuemmele left a comment

Choose a reason for hiding this comment

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

Are there UTs testing that the api version is set based on the hierarchy?

@shetzel
Copy link
Contributor Author

shetzel commented Dec 7, 2022

Are there UTs testing that the api version is set based on the hierarchy?

Those tests are in progress.

componentSet.apiVersion ??= apiversion;
componentSet.sourceApiVersion ??= sourceapiversion;
logger.debug(`ComponentSet apiVersion = ${componentSet.apiVersion}`);
logger.debug(`ComponentSet sourceApiVersion = ${componentSet.sourceApiVersion}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

checking my understanding...if I got a componentSet via componentSetBuilder, and used a manifest, then sourceApiVersion would have been set by CSB.

but not if the consumer passed in a sourceApiVersion -- then that would undo whatever the manifest version is?

So...would it make sense to disallow that behavior (like a clever union type on ComponentSetOptions that doesn't allow those props to co-exist)? Or do you think there's a use case for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sourceApiVersion set by ComponentSet.fromManifest() takes precedence. The sourceApiVersion passed in as an option to CSB would be ignored.

@@ -236,6 +244,11 @@ export class MetadataApiRetrieve extends MetadataTransfer<MetadataApiRetrieveSta
requestBody.singlePackage = this.options.singlePackage;
}

// Debug output for API version used for retrieve
const manifestVersion = manifestData.version ?? requestBody.apiVersion;
Copy link
Contributor

Choose a reason for hiding this comment

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

see above...there's definitely something in manifestData.version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're correct. This is a "suspenders and a belt" bit of code that I can modify to just use manifestData.version. I'll make that change.

result.apiVersion = manifest.apiVersion;

result.logger.debug(`Setting sourceApiVersion of ${manifest.apiVersion} on ComponentSet from manifest`);
result.sourceApiVersion = manifest.apiVersion;
result.fullName = manifest.fullName;
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: manifest resolver never returns a full name. It's optional in the return type, but never set.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not even sure what that is supposed to represent

@mshanemc
Copy link
Contributor

mshanemc commented Dec 9, 2022

QA notes:

✅ source: deploy package.xml version wins over org, for sourceApiVersion.
../../plugin-source/bin/dev force:mdapi:deploy -d mdapiOut --apiversion 54.0. I can't tell from the DEBUG what's happening with mdapi deploys. Did you try those?

❓ v52 in package.xml, 55.0 in project, and ../../plugin-source/bin/dev force:source:deploy -x mdapiOut/package.xml --apiversion 50.0. Is that supposed to work?
sf:MetadataApiDeploy DEBUG Deploying metadata source in v52.0 shape using SOAP v50.0 +22ms

no version in package.xml ../../plugin-source/bin/dev force:source:deploy -x mdapiOut/package.xml --apiversion 50.0
sf:MetadataApiDeploy DEBUG Deploying metadata source in v55.0 shape using SOAP v50.0 +26ms
✅ [it's reading the sourceApiVersion from the sfdx-project

no version in package.xml, no version in project, --apiversion 50
✅ sf:MetadataApiDeploy DEBUG Deploying metadata source in v50.0 shape using SOAP v50.0 +21ms

same, but remove the flag (both use org's version)
✅ sf:MetadataApiDeploy DEBUG Deploying metadata source in v56.0 shape using SOAP v56.0 +0ms

add 53.0 as local config
✅ sf:MetadataApiDeploy DEBUG Deploying metadata source in v53.0 shape using SOAP v53.0 +0ms

53.0 in config, --apiversion 51.0 (flag should win)
✅ sf:MetadataApiDeploy DEBUG Deploying metadata source in v51.0 shape using SOAP v51.0 +29ms

retrieves

53.0 in config, --apiversion 51.0 , 45.0 in manifest (manifest should win)
✅ sf:MetadataApiDeploy DEBUG Deploying metadata source in v51.0 shape using SOAP v51.0 +29ms

53.0 in config, --apiversion 51.0 , 53.0 in project, -p classes (project should win)
❓ sf:MetadataApiRetrieve DEBUG Retrieving source in v53.0 shape using SOAP v51.0 +8ms (still think that's weird)

remove project value (flag should win)
✅ sf:MetadataApiRetrieve DEBUG Retrieving source in v51.0 shape using SOAP v51.0 +9ms

[unset config, no flag] (org should win)
✅ sf:MetadataApiRetrieve DEBUG Retrieving source in v56.0 shape using SOAP v56.0 +0ms

add env at 49.0
✅ sf:MetadataApiRetrieve DEBUG Retrieving source in v49.0 shape using SOAP v49.0 +0ms

add 53 in project, still env 49.0
✅ project > env

add 48.0 flag, still 53 in project, still env 49.0 (flag should win on connection, but project wins on sourceApiVersion)
✅ sf:MetadataApiRetrieve DEBUG Retrieving source in v53.0 shape using SOAP v48.0 +8ms

convert

source:convert (nothing in project, no org involved). Coverage report wins?
✅ writes a v57 manifest

source:convert (53 in project, no org involved). project wins
✅ writes a v53 manifest

source:convert (52 in config).
❌ wrote a v57 manifest. Convert ignores config? Or should it follow the same conventions as deploy/retrieve?

manifest create

force:source:manifest:create -m ApexClass
👎🏻 WARNING: apiVersion configuration overridden at "52.0" (true because of config)
✅ wrote a v53 manifest because project

same command, but no value in project (expect v52 from config)
❌ writes a v57 config

@mshanemc mshanemc merged commit 2e865e9 into main Dec 9, 2022
@mshanemc mshanemc deleted the sh/api-version-fixes branch December 9, 2022 21:36
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.

4 participants