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: change mdcoverage url for getCurrentApiVersion #1191

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

shetzel
Copy link
Contributor

@shetzel shetzel commented Dec 13, 2023

What does this PR do?

Changes the URL to get an API version from https://mdcoverage.secure.force.com/services/apexrest/report to https://dx-extended-coverage.my.salesforce-sites.com/services/data
If neither sourceApiVersion nor apiVersion were set on a ComponentSet before calling getObject(), will then look for and use (in order):

  1. sourceApiVersion set in sfdx-project.json
  2. apiVersion from ConfigAggregator (config files and Env Vars)
  3. http call to /services/data endpoint for highest apiVersion
  4. hardcoded value of "58.0" as a last resort

What issues does this PR fix or reference?

@W-14621701@

@shetzel shetzel requested a review from a team as a code owner December 13, 2023 00:27
src/collections/componentSet.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

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

comment is nit.

I'd suggest recommend an FYI to VSCode team about this change--they should be doing apiVersion stuff already. I just know they've had some issues in the past around these Singleton and cwd patterns in the past when they have long-running processes. Give Pete's folks something else to test.

members: ['b', 'c'],
},
],
version: '58.0',
Copy link

Choose a reason for hiding this comment

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

why version 58.0 as default?

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 just chose a recent one. It's better than throwing an error. We really should never get to this point. ComponentSets should have apiVersion and sourceApiVersion set by the consumer.

Copy link

Choose a reason for hiding this comment

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

At some point version 58.0 will be too old and things may not function as expected on this version. In that case does it make sense to throw an error vs using a default value that is old?

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 code will warn with the missingApiVersion message should it ever get to this point and use v58.0, so if an end user sees this message they can do things to get better results should it fail. If we throw, they will definitely need to do something to get it to work. Both Shane and I felt that doing what we could to not fail is better.

In practice, the checks for an API version should never get past the sourceApiVersion and apiVersion set on the ComponentSet. If users of SDR are creating ComponentSets without setting a sourceApiVersion and/or apiVersion then they shouldn't be creating a manifest from it.

src/registry/coverage.ts Outdated Show resolved Hide resolved
};

export const getCurrentApiVersion = async (): Promise<number> => {
const apiVersionsUrl = 'https://dx-extended-coverage.my.salesforce-sites.com/services/data';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're taking that approach, maybe https://org62.my.salesforce-sites.com/services/data would be more reliable/scalable than this team's endpoint.

or use Promise.any of several known URLs

@mshanemc
Copy link
Contributor

QA (using the node REPL)

> const csb = require('./lib/src/collections/componentSetBuilder.js').ComponentSetBuilder
undefined
> const result = await csb.build({sourcepath: ['./test/nuts/local/replacements/testProj/force-app']})
undefined
> const pkg = await result.getPackageXml()
undefined
> pkg

✅ has ' <version>59.0</version>\n' +
[turns off internet]

> const pkg2 = await result.getPackageXml()
undefined
> pkg2

✅ defaulted to v58 ' 58.0\n' +

@mshanemc mshanemc merged commit ff82bb9 into main Dec 15, 2023
70 checks passed
@mshanemc mshanemc deleted the sh/switch-mdcoverage-url branch December 15, 2023 16:47
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.

5 participants