Skip to content

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

Merged
aparajit-pratap merged 12 commits intoDynamoDS:masterfrom
aparajit-pratap:customPackage
Jul 3, 2019
Merged

Prevent custom node package containing duplicate node definition from loading#9815
aparajit-pratap merged 12 commits intoDynamoDS:masterfrom
aparajit-pratap:customPackage

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

mjkkirschner commented Jun 26, 2019

@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 commented Jun 26, 2019
View reviewed changes
@aparajit-pratap aparajit-pratap added the PTAL Please Take A Look 👀 label Jun 27, 2019
@mjkkirschner
Copy link
Member

mjkkirschner commented Jun 27, 2019

@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

mjkkirschner commented Jun 27, 2019

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
mjkkirschner
mjkkirschner reviewed Jun 27, 2019
View reviewed changes
src/DynamoPackages/PackageLoader.cs
}
catch (CustomNodePackageLoadException e)
{
Package originalPackage =
Copy link
Member

@mjkkirschner mjkkirschner Jun 27, 2019

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

@QilongTang QilongTang Jun 27, 2019

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

@mjkkirschner mjkkirschner Jun 27, 2019

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

mjkkirschner commented Jun 27, 2019

@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

QilongTang commented Jul 1, 2019

LGTM

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

QilongTang commented Jul 1, 2019

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?

mjkkirschner
mjkkirschner reviewed Jul 2, 2019
View reviewed changes
@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

mjkkirschner commented Jul 5, 2019

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

@aparajit-pratap
Copy link
Contributor Author

aparajit-pratap commented Jul 8, 2019

@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

@mjkkirschner mjkkirschner mjkkirschner left review comments

@QilongTang QilongTang QilongTang 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