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.

Sign up for GitHub

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

Jump to bottom

Prevent custom node package containing duplicate node definition from loading #9815

Merged
merged 12 commits into from
Jul 3, 2019
Merged

Prevent custom node package containing duplicate node definition from loading #9815

merged 12 commits into from
Jul 3, 2019

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Jun 25, 2019 •
edited
Loading

Purpose

JIRA: DYN-1993

  • Prevent custom node package containing an existing node definition from loading (conflicting package)
  • Display "Cannot download package" dialog with "MarkForUninstall" option when trying to install conflicting package
  • Display failure to load conflicting CN package notification to notification view extension during startup (when loading already downloaded package) and also during install
  • Test these changes with @scottmitchell's fixes in Refactor PackageDependencies property #9803

image

TODO:

  • Turn package install button red on failure to install ZT and custom node packages
  • Add failure dialog similar to failed custom node package install for ZT package install as well
  • Show dialog even when opening duplicate custom node definitions

Declarations

Check these if you believe they are true

  • The code base 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.

FYIs

@mjkkirschner @scottmitchell @QilongTang

@mjkkirschner
Copy link
Member

@aparajit-pratap what is a duplicatePackage specifically?

@aparajit-pratap aparajit-pratap changed the title Prevent duplicate custom node package from loading Prevent custom node package containing duplicate node definition from loading Jun 26, 2019
@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Jun 26, 2019 •
edited
Loading

@aparajit-pratap what is a duplicatePackage specifically?

I used the term (probably misleading) to refer to a CN package that contained an already existing (duplicate) node definition. I will try to remove all usages of it to avoid confusion.

@aparajit-pratap aparajit-pratap added the PTAL Please Take A Look 👀 label Jun 27, 2019
@mjkkirschner
Copy link
Member

@Amoursol @kronz - can you chime in here about the wording of this error message - it's a complex problem but I think there might be some way to summarize it more easily or break up the text to make it look a little less imposing.

@mjkkirschner
Copy link
Member

heres my try:

{clockwork 1.x} cannot be loaded. 

Installing it will conflict with one or more node definitions that already exist in { clockwork 2.x } which is currently loaded. 

To install { clockwork 1.x }, Dynamo needs to first uninstall { clockwork 2.x }. 

Restart Dynamo to complete the uninstall, then try and download {clockwork 1.x } again.</value>

mjkkirschner
mjkkirschner reviewed Jun 27, 2019
View reviewed changes
src/DynamoPackages/PackageLoader.cs
@@ -207,13 +207,26 @@ internal void TryLoadPackageIntoLibrary(Package package)
package.Loaded = true;
this.PackgeLoaded?.Invoke(package);
}
catch (CustomNodePackageLoadException e)
{
Package originalPackage =
Copy link
Member

Choose a reason for hiding this comment

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

@QilongTang what do you think - should we log this to analytics?

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth! But are you thinking about public APIs for extention author to use or internal call only managed by our team?

Copy link
Member

Choose a reason for hiding this comment

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

am thinking about the regular log Analytics calls for events - I'm not sure if this would already be logged a custom exception type or we only log uncaught exceptions?

@mjkkirschner
Copy link
Member

@aparajit-pratap is it very difficult to add a test to assert the second package is not allowed to load?

@aparajit-pratap aparajit-pratap mentioned this pull request Jul 1, 2019
Merged
7 tasks
@QilongTang
Copy link
Contributor

LGTM

@QilongTang QilongTang added LGTM Looks good to me and removed PTAL Please Take A Look 👀 labels Jul 1, 2019
@QilongTang
Copy link
Contributor

Seems UI behavior is not covered in the unit test here, should we create a Jira task to cover this in the UI automation pipeline?

@aparajit-pratap aparajit-pratap merged commit ef55f1b into DynamoDS:master Jul 3, 2019
@aparajit-pratap aparajit-pratap deleted the customPackage branch July 3, 2019 12:48
@aparajit-pratap aparajit-pratap restored the customPackage branch July 3, 2019 12:48
@aparajit-pratap aparajit-pratap deleted the customPackage branch July 3, 2019 13:53
aparajit-pratap added a commit that referenced this pull request Jul 3, 2019
@aparajit-pratap
Cherry-pick #9815 (#9821)
cdcf966 
* fix for adding duplicate ZT search entries for packages already loaded

* added test

* add comments to test

* prevent duplicate custom node package from loading

* rename

* rename

* code fixes

* code cleanup

* update wording of custom node package fail dialog

* address review comments

* add test custom node packages with test

* fix failing tests

* fix test
@mjkkirschner
Copy link
Member

@aparajit-pratap - what happened with the TODOs - were followups filed?

@aparajit-pratap
Copy link
Contributor Author

@mjkkirschner thanks for reminding. I intended to file them as separate tasks. Will do.

mjkkirschner pushed a commit that referenced this pull request Aug 3, 2019
@aparajit-pratap @mjkkirschner
Prevent custom node package containing duplicate node definition from…
bc7d750 
… loading (#9815)

* prevent duplicate custom node package from loading

* rename

* rename

* code fixes

* code cleanup

* update wording of custom node package fail dialog

* address review comments

* add test custom node packages with test

* fix failing tests

* fix test
@aparajit-pratap aparajit-pratap mentioned this pull request Sep 9, 2019
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QilongTang QilongTang QilongTang left review comments

@mjkkirschner mjkkirschner mjkkirschner left review comments

Assignees
No one assigned
Labels
LGTM Looks good to me
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aparajit-pratap @mjkkirschner @QilongTang