DYN-6901 Pm - retainfolderstructure rework #15357
Conversation
- plotting out the publishing process with series of tests
- previously dynamo would add dyf and dll files regardless of the capitalization of the extesion. However - however, when copying the files this was not respected - now also copies the files awith upper capitalization
- fixed caps extension issue in preview and remove
- confirm that the original custom definition files will be deleted during the build process - fixed a few more extension capitalizations
- rework on the way the content structure is recreated (files and folders relationships) - additional test coverage to assert edge case scenarios
- rework of most of the logic behind retaining folder structure - fixing tests
- we cannot really allow to keep the existing pkg.json file because that will invalidate the form filling part of the process
…tructure-updates-rework
…ructure-updates-rework
- this was a mix-up of the old way of displaying custom nodes and the new ones only
|
From our discussion in slack, lets remove the code that deletes |
dnenov
changed the title
- now correctly keeps single folder structure while allowing for correct nesting when multiple distinct root folders are present
…tructure-updates-rework
UI Smoke TestsTest: success. 11 passed, 0 failed. |
- added more test coverage to capture different scenarios - adapted the remapretaindyf function for edge case scenarios where dyf files reside in the root folder (losing the folder/file structure)
…ructure-updates-rework
…etainfolderstructure-updates-rework
| @@ -94,15 +94,15 @@ public PackageUpload NewPackageUpload(Package package, string packagesDirectory, | |||
| /// <param name="handle"></param> | |||
| /// <returns></returns> | |||
| /// <exception cref="ArgumentNullException"></exception> | |||
| public PackageUpload NewPackageRetainUpload(Package package, string packagesDirectory, IEnumerable<IEnumerable<string>> files, IEnumerable<string> markdownFiles, PackageUploadHandle handle) | |||
| public PackageUpload NewPackageRetainUpload(Package package, string packagesDirectory, IEnumerable<string> roots, IEnumerable<IEnumerable<string>> files, IEnumerable<string> markdownFiles, PackageUploadHandle handle) | |||
There was a problem hiding this comment.
@QilongTang is it OK to break public API here? I know "retain" is a newer API and may not be leveraged by many users currently. Just wanted to start this discussion.
We can either override or make a note in the next release notes.
There was a problem hiding this comment.
We probably want to override with a new version and use that one instead, thank you for pointing it out
There was a problem hiding this comment.
Oh, of course! I did not think about the API breaking change here, thank you for pointing it out @zeusongit . I will try to sort that out.
There was a problem hiding this comment.
I recreated the obsolete logic chain to prevent breaking API, marking it [Obsolete]. I don't think I had another option but to have the whole of the old methods in, as the signature was being passed onwards continuously.
src/DynamoCoreWpf/ViewModels/PackageManager/PublishPackageViewModel.cs
Outdated
Show resolved
Hide resolved
|
Have I been included as an FYI in error? |
I am guessing this is meant to FYI: @Amoursol but when typing |
Apologies, I must have looped you in by mistake! |
- recreated the obsolete logic chain
|
Hi @dnenov , |
Ooh, ok! I know what you mean @zeusongit! Would you mind, would it be OK if I targeted this with another PR? It is a tricky one, and I would not like to slow this PR down if possible. From memory, the flow at the moment forces you to go to the next page. Or, in other words, we navigate away from this page the moment we hit the 'load' button, and we check the result 'on the other side'. This improvement would mean moving the logic over to the page, so we can first check the dialog result and then navigate away. |
Sounds good 👍🏽 |
zeusongit
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-6901
…ructure-updates-rework
- it seems like a package is loaded in the test environment. added a fail message to try and locate that package
|
I fixed the failing tests which this PR was responsible for. I am trying to figure out why there is a failing test which indicates that there are packages installed under the test environment. |
|
It looks like everything is passing now. |
Purpose
Based on #15314, this PR completes the changes to the
retain folder structurelogic. Here we are making sure that the 'preview' of the package produces the same result.The change focuses around the idea that we have 2 main file/folder structures.
Single root folder
The problem here was that when using a single folder structure, we would nest the origin into the newly created package folder. This was causing confusion for authors that wanted to keep their original folder structure 'just as it is'.
Multiple root folders
In this scenario, we have 2 or more 'roots' - this can happen for various reasons, for example if we load files from 2 separate drive letters (C:\ and D:).
Here we have no other option but to nest all of the roots into the newly created package folder.
Legacy code review - delete dyf files
Added a test to highlight an existing behavior where Dynamo will delete all
Custom Nodesduring the build process. This side effect is unexpected and not communicated to the user, which could lead to issues.BuildPackageDirectory_DeletesOriginalDyfFiles()dyfswhen building, there isn't anything immediately obvious as a negative consequence. Furthermore, we do have a guardrails in the code to prevent from publishing (and loading) custom nodes already under package control:private IEnumerable<string> GetAllUnqualifiedFiles()@QilongTang @zeusongit @Amoursol
Known limitations - skip empty folders (should we keep or do we want to change?)
Currently, we only 'care' about files. As a consequence, we will skip empty folders.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
PackageDirectoryBuilderto pass on the 'roots' information so the build logic can correctly dispatch the files/folders matching the previewReviewers
@QilongTang
@zeusongit
@reddyashish
FYIs
@Amoursol