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 Civil3D hostname to fix psuedo warning for package users #13910

Merged
merged 7 commits into from
Apr 24, 2023

Conversation

zeusongit
Copy link
Contributor

Purpose

https://jira.autodesk.com/browse/DYN-5470
Added Dynamo Civil 3D as the hostname for Civil 3D packages that have Civil 3D as the host dependency.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

Reviewers

@DynamoDS/dynamo

/// as there is a mismatch between the host dependency name in packages and the host name used by Civil 3D for Dynamo.
/// </summary>
private readonly string Civil3DHostName = "Dynamo Civil 3D";
private readonly string oldCivil3DHostName = "Civil 3D";
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm it's a bit weird to define this in the Dynamo code base, should we switch to a more tolerant match based on your earlier comment?

Copy link
Member

@mjkkirschner mjkkirschner Apr 19, 2023

Choose a reason for hiding this comment

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

just curious - could we do this transformation at runtime on the package manager? (not actually modify the package entries on disk) ... I prefer this because we don't need to update dynamo to fix the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjkkirschner Do you mean adding the hostname in the hosts returned from Package Manager?
here: https://git.autodesk.com/Dynamo/PackageManager/blob/c1e6047b5a92419049098c4a478bfb463aed6aae/src/app.js#L239

Copy link
Contributor Author

@zeusongit zeusongit Apr 19, 2023

Choose a reason for hiding this comment

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

hmm it's a bit weird to define this in the Dynamo code base, should we switch to a more tolerant match based on your earlier comment?

@QilongTang updated with more tolerant approach

Copy link
Member

Choose a reason for hiding this comment

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

no, I mean that when package manager encounters a package with this old hostname to just return the new one instead in the response when the user downloads this package or gets its header... but perhaps this has issues with the pkg.json?

@QilongTang QilongTang added this to the 2.18.0 milestone Apr 21, 2023
@reddyashish
Copy link
Contributor

looks like a sporadic failure. Running the job again: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/8921/

@QilongTang QilongTang merged commit 093f5a5 into DynamoDS:master Apr 24, 2023
@reddyashish
Copy link
Contributor

Cherrypick this?

zeusongit added a commit to zeusongit/Dynamo that referenced this pull request Apr 25, 2023
…S#13910)

* add cache

* add new host name check

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs
QilongTang pushed a commit that referenced this pull request Apr 25, 2023
…#13935)

* add cache

* add new host name check

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs

* Update PackageManagerExtension.cs
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