Skip to content

DYN-10096: On package install engine_version checks are suppressed if compatibility_matrix check passes#17173

Open
ivaylo-matov wants to merge 3 commits into
DynamoDS:masterfrom
ivaylo-matov:DYN-10096-On-package-install-engine_version-checks-are-suppressed-if-compatibility_matrix-check-pass
Open

DYN-10096: On package install engine_version checks are suppressed if compatibility_matrix check passes#17173
ivaylo-matov wants to merge 3 commits into
DynamoDS:masterfrom
ivaylo-matov:DYN-10096-On-package-install-engine_version-checks-are-suppressed-if-compatibility_matrix-check-pass

Conversation

@ivaylo-matov

Copy link
Copy Markdown
Contributor

Purpose

This PR fixes DYN-10069, where Clockwork installation on Dynamo 3.6.1 incorrectly triggers engine version warnings despite valid compatibility.

Root cause is in PackageManagerClientViewModel.ExecutePackageDownload(), which evaluates both compatibility_matrix and published engine_version. The engine version check can override valid compatibility results.

Fix:

  • made compatibility_matrix the primary compatibility source
  • only fall back to engine_version when compatibility is unknown or incompatible
  • removed hardcoded Dynamo 4.x assumption in engine version logic
  • generalized warning to any dependency with a lower major engine version than the current Dynamo runtime
  • added unit test to ensure no warnings are raised when compatibility_matrix reports compatibility

Declarations

Check these if you believe they are true

Release Notes

Fixes incorrect package compatibility warnings during installation when packages are marked compatible via the compatibility matrix.

Reviewers

@DynamoDS/eidos
@jasonstratton

FYIs

@johnpierson
@dnenov

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-10096

@ivaylo-matov ivaylo-matov changed the title DYN-10096: On-package-install-engine_version-checks-are-suppressed-if-compatibility_matrix-check-pass DYN-10096: On package install engine_version checks are suppressed if compatibility_matrix check passes Jun 18, 2026
…n-checks-are-suppressed-if-compatibility_matrix-check-pass

@jasonstratton jasonstratton left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Address the two comments and I think it's good. ... Just let me know when you're done in order to merge.

try
// If the compatibility matrix already indicates compatibility, skip the engine_version heuristic.
// Only perform engine_version based warnings when compatibility is not explicitly true.
if (compatible == false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be if (compatible != true)? ... There are actually 3 states, true, false and null


[Test]
[Description("If the package compatibility matrix explicitly marks the package as compatible, do NOT show engine_version newer/older warnings even when engine_version differs.")]
public void PackageManagerDoesNotShowEngineVersionWarningWhenCompatibilityMatrixIsCompatible()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tests the happy path, please add a test for the null case (no matrix → warning should still appear)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PackageManagerDoesNotShowEngineVersionWarningWhenCompatibilityMatrixIsCompatible now added to test the null case

@sonarqubecloud

Copy link
Copy Markdown

@ivaylo-matov

Copy link
Copy Markdown
Contributor Author

Address the two comments and I think it's good. ... Just let me know when you're done in order to merge.

Thanks for the review, @jasonstratton ! I've addressed both comments, the PR should be ready to merge.

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.

2 participants