DYN-5709: Pm publish version patch request #15395

dnenov · 2024-07-18T08:16:18Z

Purpose

An urgent request to 'fix' the Publish version experience.

historically, when publishing a new package or a new package version, we would re-build the package anew every time. This creates a number of issues around tempering with loaded resources (removing, overriding, loading, unloading)
we make an assumption that if a package is installed, the user wants to publish (either as a brand new package, or a version) the actual installed package with its existing files and folder structure
this means that we disable the option to add/remove files when publishing from locally installed package
which allows us to simply zip the existing files and folders package structure, only amending the Header file as needed

For review. Needs testing.

User experience

Installed Packages

Edit package details (header); Publish Locally is disabled

'Edit' is still available to preview the package structure, but editing options are disabled

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

now Publish installed package also uses RetainFolderStructure
both Publish and Publish version now mark the package as installed
if a package is installed (IsPackageInstalled) then we bypass the standard build procedure, and use (zip) the existing package folder
if a package is installed the editing functionalities of the publish package workflow are disabled, ie. we cannot add/remove files (as this will require a rebuild)
if a package is installed we don't allow local installation, only publish online (as the package is already installed after all?)
also allows local package to be published only after it has been created (merged DYN-5709 jira request, as it overlaps a bit)

Reviewers

@aparajit-pratap
@mjkkirschner

FYIs

@QilongTang
@zeusongit
@reddyashish

- changed publish package command restrictions - located the place where dll files would cause error on deletion

- dll being loaded doesn't seem to be a problem

…on-patch-request

- historically, when publishing a new package or a new package version, we would re-build the package anew every time. This creates a number of issues around tempering with loaded resources (removing, overriding, loading, unloading) - we make an assumption, that if a package is installed, the user wants to publish (either as a brand new package, or a version) the actual installed package with its existing files and folder structure - this means that we disable the option to dynamically add/remove files if we publish from locally installed package - which allows us to simply zip the existing folder structure, only amending the Header file as needed

github-actions

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

github-actions · 2024-07-18T08:41:55Z

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

aparajit-pratap · 2024-07-18T17:40:16Z

if a package is installed we don't allow local installation, only publish online (as the package is already installed after all?)

@dnenov instead of disabling publish locally can we still keep it enabled and instead disable reloading the package after it has been published? This helps a lot with testing whether your package will eventually be published online correctly.

aparajit-pratap · 2024-07-18T17:59:40Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageViewModel.cs

+        // If the current user is the package holder and a package with that name has not been published yet, return true
+        private bool PackageNotPublishedAndUserIsHolder(Package package, string username)
+        {
+            bool userIsHolder = package.CopyrightHolder != null && package.CopyrightHolder.Equals(username);


Is the copyright information coming from the pkg.json file? What if it's missing?

How about renaming this function as IsPackageUnpublishedAndUserHolder?

aparajit-pratap · 2024-07-18T19:07:44Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageViewModel.cs

+        {
+            if (!CanPublish) return false;
+
+            return packageManagerClient.DoesCurrentUserOwnPackage(Model, dynamoModel.AuthenticationManager.Username) ||


~~Does the DoesCurrentUserOwnPackage check mean that the package cannot be republished by someone who hasn't published it before?~~

~~It looks like only an existing package maintainer or package copyright holder can republish (publish a new version) the package? The code would be clearer if you said something like:~~

bool isUserOwnerOrHolder = packageManagerClient.DoesCurrentUserOwnPackage(Model, dynamoModel.AuthenticationManager.Username) || PackageNotPublishedAndUserIsHolder(Model, dynamoModel.AuthenticationManager.Username); return isUserOwnerOrHolder;

Wait..Copyright Holder should not be able to publish a version or a package, we do not validate copyright holder on server side, any actions related to package publishing either republishing from local copy or publishing a version should be limited to its current maintainers only.

Understood.

aparajit-pratap · 2024-07-18T19:14:34Z

src/DynamoCoreWpf/ViewModels/PackageManager/PackageViewModel.cs

            vm.IsNewVersion = false;
+            vm.IsPackageInstalled = true;


Will this always be true? In the case of publishing a new package, the new package doesn't need to always be loaded, does it?

aparajit-pratap · 2024-07-18T19:20:32Z

src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs

@@ -2117,9 +2136,9 @@ private void RemoveSingleItem(PackageItemRootViewModel vm, DependencyType fileTy
            }
        }

-        private bool CanShowAddFileDialogAndAdd()
+        private bool CanAddFiles()


Why not just remove this redundant property (it's just the inverse of IsPackageInstalled) and use IsPackageInstalled directly?

zeusongit · 2024-07-18T20:29:44Z

this means that we disable the option to add/remove files when publishing from locally installed package

what is the process to include new version of the existing dll in your package, or removing existing ones?

aparajit-pratap · 2024-07-18T22:46:16Z

@dnenov I was able to publish the DynamoForma package with these changes 🎉 Thank you!

A couple of comments:

To @zeusongit's point, I now understand why the publish version workflow was designed the way it is today:
I was unable to mark files as node libraries in the edit UI. This would need more discussion/design as it could mean that the new package isn't loaded properly in Dynamo in the first place without the node libraries updated.

One workaround for this could be that we require authors to manually edit the pkg.json file with the newly updated node libraries if they want to publish the package while being loaded correctly. We then completely disable editing node library checks in the UI, we simply display them as read-only. This, however, is not an intuitive option and difficult to message/make discoverable to package authors.

On second thoughts I think it's best to revert to the old publish version workflow of allowing adding/removing files (including node library tagging), then publishing the edited package but not reloading it in the same Dynamo session. We could simply message that users would need to restart Dynamo to load the edited package.

Thoughts @zeusongit?

aparajit-pratap · 2024-07-18T22:48:32Z

this means that we disable the option to add/remove files when publishing from locally installed package

what is the process to include new version of the existing dll in your package, or removing existing ones?

@zeusongit when @dnenov and I discussed this, we found that it makes it very complicated to add or remove files to an already loaded package and then republish it as a new version/package (I think because the new package post-edits is then loaded in the same Dynamo session). We therefore decided to simplify this workflow by disallowing file addition or removal while publishing an already installed package. If this is required, the idea is for the package author to have the package in its final, ready-to-publish state already loaded in Dynamo before publishing it.

However, this new approach has its disadvantages as well. Please see my comment above.

dnenov · 2024-07-19T13:11:54Z

Hi @aparajit-pratap @QilongTang. Thank you for your comments. I was hoping that this line of simplification might be a good one, but it might turn out that it's not.

Here are some of my thoughts:

On the technical side, I am mostly concerned with manipulations around loaded assemblies. Basically, the moment a package is loaded, we really don't want to publish it anymore. Which opens a few questions - when is a package being loaded. I actually don't know the answer to that question, and need to check. If a package is loaded the moment Dynamo starts .. then how are we ever going to publish an installed package? (I use 'publish' to mean 'submit online' here).

Actually, would it be OK if we had a call around a few more questions?

zeusongit · 2024-07-19T16:45:20Z

@dnenov I was able to publish the DynamoForma package with these changes 🎉 Thank you!

A couple of comments:

To @zeusongit's point, I now understand why the publish version workflow was designed the way it is today: I was unable to mark files as node libraries in the edit UI. This would need more discussion/design as it could mean that the new package isn't loaded properly in Dynamo in the first place without the node libraries updated.

One workaround for this could be that we require authors to manually edit the pkg.json file with the newly updated node libraries if they want to publish the package while being loaded correctly. We then completely disable editing node library checks in the UI, we simply display them as read-only. This, however, is not an intuitive option and difficult to message/make discoverable to package authors.

On second thoughts I think it's best to revert to the old publish version workflow of allowing adding/removing files (including node library tagging), then publishing the edited package but not reloading it in the same Dynamo session. We could simply message that users would need to restart Dynamo to load the edited package.

Thoughts @zeusongit?

Agreed, second option makes sense. But I can think of some ways that may be a problem, like adding binaries which are unchanged and already exist.

dnenov · 2024-07-22T15:01:38Z

Parking this one after discussion with @zeusongit and @QilongTang. Will pursue a proper context isolation per plugin as a way to move forward.

aparajit-pratap · 2024-07-23T18:30:41Z

I can think of some ways that may be a problem, like adding binaries which are unchanged and already exist.

@zeusongit why would that be a problem?

dnenov · 2024-07-24T15:07:46Z

Opening up as a temp solution to the currently broken Publish version workflow.

…n-patch-request

QilongTang · 2024-07-26T19:32:10Z

@dnenov This PR is no longer required right?

dnenov added 4 commits July 14, 2024 13:10

initial commit

8fc7e96

- changed publish package command restrictions - located the place where dll files would cause error on deletion

dll loading not a problem

d1bbc5e

- dll being loaded doesn't seem to be a problem

Merge branch 'DYN-5709-local-to-online-package' into pm-publish-versi…

2c0eff3

…on-patch-request

github-actions bot changed the title ~~Pm publish version patch request~~ DYN-5709: Pm publish version patch request Jul 18, 2024

github-actions bot reviewed Jul 18, 2024

View reviewed changes

QilongTang added this to the 3.3 milestone Jul 18, 2024

aparajit-pratap reviewed Jul 18, 2024

View reviewed changes

dnenov closed this Jul 22, 2024

dnenov reopened this Jul 24, 2024

dnenov added wontfix WIP labels Jul 24, 2024

dnenov marked this pull request as draft July 24, 2024 15:08

Merge remote-tracking branch 'upstream/master' into pm-publish-versio…

9fef12d

…n-patch-request

QilongTang removed this from the 3.3 milestone Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-5709: Pm publish version patch request #15395

DYN-5709: Pm publish version patch request #15395

dnenov commented Jul 18, 2024 •

edited

Loading

github-actions bot left a comment

github-actions bot commented Jul 18, 2024 •

edited

Loading

aparajit-pratap commented Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024

aparajit-pratap Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024 •

edited

Loading

zeusongit Jul 18, 2024

dnenov Jul 19, 2024

aparajit-pratap Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024

zeusongit commented Jul 18, 2024 •

edited by aparajit-pratap

Loading

aparajit-pratap commented Jul 18, 2024

aparajit-pratap commented Jul 18, 2024 •

edited

Loading

dnenov commented Jul 19, 2024

zeusongit commented Jul 19, 2024 •

edited

Loading

dnenov commented Jul 22, 2024

aparajit-pratap commented Jul 23, 2024

dnenov commented Jul 24, 2024

QilongTang commented Jul 26, 2024

DYN-5709: Pm publish version patch request #15395

Are you sure you want to change the base?

DYN-5709: Pm publish version patch request #15395

Conversation

dnenov commented Jul 18, 2024 • edited Loading

Purpose

User experience

Declarations

Release Notes

Reviewers

FYIs

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2024 • edited Loading

UI Smoke Tests

aparajit-pratap commented Jul 18, 2024 • edited Loading

aparajit-pratap Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Jul 18, 2024

Choose a reason for hiding this comment

aparajit-pratap Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

zeusongit Jul 18, 2024

Choose a reason for hiding this comment

dnenov Jul 19, 2024

Choose a reason for hiding this comment

aparajit-pratap Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Jul 18, 2024

Choose a reason for hiding this comment

zeusongit commented Jul 18, 2024 • edited by aparajit-pratap Loading

aparajit-pratap commented Jul 18, 2024

aparajit-pratap commented Jul 18, 2024 • edited Loading

dnenov commented Jul 19, 2024

zeusongit commented Jul 19, 2024 • edited Loading

dnenov commented Jul 22, 2024

aparajit-pratap commented Jul 23, 2024

dnenov commented Jul 24, 2024

QilongTang commented Jul 26, 2024

dnenov commented Jul 18, 2024 •

edited

Loading

github-actions bot commented Jul 18, 2024 •

edited

Loading

aparajit-pratap commented Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024 •

edited

Loading

aparajit-pratap Jul 18, 2024 •

edited

Loading

zeusongit commented Jul 18, 2024 •

edited by aparajit-pratap

Loading

aparajit-pratap commented Jul 18, 2024 •

edited

Loading

zeusongit commented Jul 19, 2024 •

edited

Loading