Skip to content

DYN-4121: Publishing a package copies it to root directory instead of /packages so it never loads. #12081

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
Merged
QilongTang merged 1 commit into from
Sep 30, 2021
Merged

DYN-4121: Publishing a package copies it to root directory instead of /packages so it never loads. #12081

QilongTang merged 1 commit into from
Sep 30, 2021

Conversation

@sm6srw
Copy link
Contributor

@sm6srw sm6srw commented Sep 28, 2021 •
edited
Loading

Purpose

This pull request does:

  • remove outdated logic

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

@sm6srw sm6srw added the WIP label Sep 28, 2021
@sm6srw sm6srw changed the title Remove outdated logic DYN-4121: Publishing a package copies it to root directory instead of /packages so it never loads. Sep 28, 2021
sm6srw
sm6srw commented Sep 29, 2021
View reviewed changes
src/DynamoPackages/PackageManagerExtension.cs
{
packageUploadDirectory = startupParams.PathManager.DefaultPackagesDirectory;
}
var packageUploadDirectory = startupParams.PathManager.DefaultPackagesDirectory;
Copy link
Contributor Author

@sm6srw sm6srw Sep 29, 2021

Choose a reason for hiding this comment

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

This logic is outdated and not needed anymore. DefaultPackagesDirectory will gives what we want in all cases.

@sm6srw sm6srw removed the WIP label Sep 29, 2021
mjkkirschner
mjkkirschner approved these changes Sep 29, 2021
View reviewed changes
Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

LGTM

@aparajit-pratap
Copy link
Contributor

aparajit-pratap commented Sep 29, 2021

Ok, looks like I broke it. 😢
LGTM!

@QilongTang QilongTang merged commit e131e64 into DynamoDS:master Sep 30, 2021
@QilongTang
Copy link
Contributor

QilongTang commented Sep 30, 2021

Merging to see if master build will be unblocked. I suspect the excel failures still require manual fixes

M-JULIANI pushed a commit to M-JULIANI/Dynamo that referenced this pull request Sep 30, 2021
@sm6srw @M-JULIANI
Remove outdated logic (DynamoDS#12081)
11a1dff
@QilongTang
Copy link
Contributor

QilongTang commented Sep 30, 2021 •
edited
Loading

Even after merging I still see the following regression on recent master-15 build, do we need to touch the build machine or there are other causes?

1) Failed : Dynamo.Tests.CrashReportingTests.NoLoadedPackages
  Expected: <empty>
  But was:  < <Dynamo.PackageManager.Package> >
   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at Dynamo.Tests.CrashReportingTests.NoLoadedPackages() in E:\Builds\Dynamo_master\Dynamo\test\DynamoCoreWpfTests\CrashReportingTests.cs:line 154

2) Failed : DynamoCoreWpfTests.PackageManagerUITests.PackageManagerLoadManuallyUnloadedBuiltIn
  Expected: 1
  But was:  2
   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at DynamoCoreWpfTests.PackageManagerUITests.PackageManagerLoadManuallyUnloadedBuiltIn() in E:\Builds\Dynamo_master\Dynamo\test\DynamoCoreWpfTests\PackageManagerUITests.cs:line 437

3) Failed : DynamoCoreWpfTests.PackageManagerUITests.PackageManagerNoDuplicatesOnPathIsRemovedThenAdded
  Expected: 0
  But was:  1
   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at DynamoCoreWpfTests.PackageManagerUITests.PackageManagerNoDuplicatesOnPathIsRemovedThenAdded() in E:\Builds\Dynamo_master\Dynamo\test\DynamoCoreWpfTests\PackageManagerUITests.cs:line 488

4) Failed : DynamoCoreWpfTests.PackageManagerUITests.PackageManagerUninstallCommand
  Expected: 1
  But was:  2
   at NUnit.Framework.Assert.That(Object actual, IResolveConstraint expression, String message, Object[] args)
   at DynamoCoreWpfTests.PackageManagerUITests.PackageManagerUninstallCommand() in E:\Builds\Dynamo_master\Dynamo\test\DynamoCoreWpfTests\PackageManagerUITests.cs:line 380

@sm6srw sm6srw deleted the DYN-4121 branch September 30, 2021 11:49
QilongTang added a commit that referenced this pull request Oct 4, 2021
@M-JULIANI @filipeotero
@SHKnudsen @saintentropy @sm6srw @aparajit-pratap @QilongTang @OliverEGreen
Connector context menu lazy load (#12091)
a8f0b95 
* Polyline + thick outline removed.

* Removed 'ConnectorType` menu from view->connectors.

* Updated connector styles.

* New resources added/old ones removed. Minor UI updates to reflect mockups.

* First commit.

* Cleanup. On-the-fly ContextMenu working.

* Applying ContextMenu styling.

* Added (stole) ContextMenu styling.

* Added resource strings to ContextMenu control.

* Update AssemblySharedInfo.cs

* Unused ref removal.

* Cleanup.

* Fixes.

* First commit.

* Cleanup. On-the-fly ContextMenu working.

* Applying ContextMenu styling.

* Added (stole) ContextMenu styling.

* Added resource strings to ContextMenu control.

* Update AssemblySharedInfo.cs

* Cleanup.

* Adding 10 to margin (#12076)

* Update AnnotationModel.cs (#12086)

* Crash Fix.  Add visibility trigger to empty tooltip strings. (#12069)

* Add visiblity trigger to empty tooltip strings

* PR comment

* Pr comments

Co-authored-by: Craig Long <craig.long@autodesk.com>

* DYN-3992: Upgrade LibG (#12060)

* mark test as failure temporarily to fix build issues (#12090)

* add pins to groups (#12070)

* add pins to groups

* Update ConnectorPinView.xaml.cs

* Remove outdated logic (#12081)

* Wire connector UI modifications (#12072)

* Polyline + thick outline removed.

* Removed 'ConnectorType` menu from view->connectors.

* Updated connector styles.

* New resources added/old ones removed. Minor UI updates to reflect mockups.

* [DYN-4105] Make sure nodes gets added to parent group (#12084)

* make sure nodes gets added to parent group

* remove commented code

Co-authored-by: Aaron (Qilong) <173288704@qq.com>

* Updates resource strings from Remove to Delete (#12088)

* Improved ContextMenu behaviour.

* PR fix.

* Removed unnecessary refs.

* Added unit tests for ConnectorContextMenu construction/disposal/commands.

Co-authored-by: filipeotero <89042471+filipeotero@users.noreply.github.com>
Co-authored-by: Sylvester Knudsen <sylvesterknudsen@gmail.com>
Co-authored-by: Craig Long <craig.alan.long@gmail.com>
Co-authored-by: Craig Long <craig.long@autodesk.com>
Co-authored-by: Jorgen Dahl <sm6srw@users.noreply.github.com>
Co-authored-by: aparajit-pratap <aparajit.pratap@autodesk.com>
Co-authored-by: Aaron (Qilong) <173288704@qq.com>
Co-authored-by: Ollie Green <oliveredwardgreen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjkkirschner mjkkirschner mjkkirschner approved these changes

@aparajit-pratap aparajit-pratap aparajit-pratap approved these changes

@pinzart pinzart Awaiting requested review from pinzart

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sm6srw @aparajit-pratap @QilongTang @mjkkirschner