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

Sm/max-api-version-on-deploys #684

Merged
merged 3 commits into from
Aug 8, 2022
Merged

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented Aug 4, 2022

What does this PR do?

  1. when the componentSet has made it to the SDR deploy step without any apiVersion info, set it from what's on the connection so it doesn't default to highest per mdapi coverage in the package.xml being sent
  2. getConnection will use org's max, emiting a warning if the user is trying to use something too high, in case the default isn't available yet to avoid issues on other endpoints polling/cancel/etc

[NB the proper way to do this is to set sourceApiVersion in your sfdx-project.json. This is the workaround for tooling to use more intelligent default behavior.

What issues does this PR fix or reference?

@W-11549415@
forcedotcom/cli#1656

Functionality Before

=== Component Failures [1]

 Type  Name        Problem                        
 ───── ─────────── ────────────────────────────── 
 Error package.xml Invalid version specified:56.0 

Functionality After

pushes as expected

@mshanemc mshanemc requested review from a team as code owners August 4, 2022 23:24
@mshanemc mshanemc requested a review from RitamAgrawal August 4, 2022 23:24
@@ -269,3 +269,16 @@ export abstract class MetadataTransfer<Status extends MetadataRequestStatus, Res
protected abstract pre(): Promise<AsyncResult>;
protected abstract post(result: Status): Promise<Result>;
}

/* prevent requests on apiVersions higher than the org supports */
const getConnectionNoHigherThanOrgAllows = async (conn: Connection, requestedVersion: string): Promise<Connection> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have test coverage for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's being executed on every deploy, so about 100x.
At least some of those specify an apiVersion...not sure if any don't and would check the if conditional.

const getConnectionNoHigherThanOrgAllows = async (conn: Connection, requestedVersion: string): Promise<Connection> => {
// uses a TTL cache, so mostly won't hit the server
const maxApiVersion = await conn.retrieveMaxApiVersion();
if (requestedVersion && parseInt(requestedVersion, 10) > parseInt(maxApiVersion, 10)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worthwhile to validate the requested version here? It seems the caller could send an invalid version string here and get by without altering the max version allowed on the connection.

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 ComponentSet allows consumers to set its apiVersion and sourceApiVersion properties. If there's a place to put validation, it'd be there. This is internal non-exported function, only called from this file, which is extended by Deploy and Retrieve.

Are you wanting to put getters/setters so we can do validation on those props for ComponentSet?

Copy link
Contributor

@gbockus-sf gbockus-sf left a comment

Choose a reason for hiding this comment

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

a few question in there but change looks good.

@shetzel shetzel merged commit 8d81800 into main Aug 8, 2022
@shetzel shetzel deleted the sm/max-api-version-on-deploys branch August 8, 2022 20:02
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