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.

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

Closes Issue#2885: Allow multiple DesktopModules in a single package #2887

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

kestasjk
Copy link
Contributor

@kestasjk kestasjk commented Jul 9, 2019

Please refer to Issue #2885

Also PR #2886 2886 is the same as this, but was originally made against a 9.3.2 branch causing a minor conflict. This PR has been closed in favor of this one.

Summary

A very simple change to the DesktopModule installation logic allowing multiple DesktopModules in a single Package.

…on set by the install manifest, instead of using the Package FriendlyName and Description, allowing multiple DesktopModules per Package.
@valadas
Copy link
Contributor

valadas commented Jul 9, 2019

Thanks @kestasjk , this one looks fine now.
@dnnsoftware/tag As far as I can see, this would be backwards compatible. Any potential risk with this PR ?

@mitchelsellers
Copy link
Contributor

Overall, I think this is a good change, however, this is a big breaking change from an expectations perspective. If a user did something different in the DesktopModule than they had in the package, and from what I can tell this could be impacting a number of existing modules.

I'd like to get a few other comments on this one.

@valadas
Copy link
Contributor

valadas commented Jul 9, 2019

And for the issue use case, this could be accomplished by having multiple packages in the manifest. One package would have the dll and the other packages the various desktopmodules or is there something I am not understanding of the use case @kestasjk .

Another issue is the documentation (and probably the installer code and the extension validation service) state that there can by only one module per package, see https://www.dnnsoftware.com/wiki/module-component

So changing this would involve a documentation change and a EVS change on top of not breaking the uninstaller and existing module packages.

Still like the idea, we just need to properly test and modify the other parts that go with this change if needed.

@mitchelsellers
Copy link
Contributor

Oh and @kestasjk I want to be 100% clear here, I LOVE this improvement, and I think it is a great contribution so please don't take any of this discussion the wrong way.

One thought that I have, do we support a bump of the Package version from the current 5.0 to say version 6 or something? And introduce the behavior change by opting into the new version?

All of @valadas points are still other areas that need adjustment, but this might make the behavior change better.

@valadas
Copy link
Contributor

valadas commented Jul 9, 2019

If we do bump the manifest version, then I say we make the manifest filename .dnn10

The idea of having .dnn5 and .dnn6 manifests was to know if the manifest was legacy or new format, but keeping the same code to handle .dnn, .dnn5 or .dnn6 meant that it was useless and most developers just use .dnn

We could do better and have .dnn.10.xml or something where we could grab just the version number and throw warning for deprecated manifests and at one time in the future refuse old formats and cleanup old install code in the platform.

With this scheme we could finally do breaking changes to the installer code and manifest schema by providing a legacy installer for old versions and a new one for the modern ones. It has been incredibly difficult to do changes in the installer because there is so much stuff to think about for old modules.

One of the things that always bugged me is how we cannot validate a manifest using and xsd because the component type is a parameter instead of an xml node. Fixing this would mean we could have intellisense in the manifests with live validation that it matches the given schema. That would allow deprecating older manifest schemas at some point in time by their filename.

And yeah, i join Mitchel to say I love the idea, just we need to be cautious and make sure we don't break stuff by improving this. And my rambling here is just me thinking out loud about related improvements. There is quite a few people here that want to improve the installer, it is just a scary best to touch. :)

@bdukes
Copy link
Contributor

bdukes commented Jul 9, 2019

I think this is a completely backwards compatible change to the installer, I don't see the benefit in bumping the manifest version number.

I would, however, be concerned about assumptions made in the Edit Bar or other components that don't expect multiple desktop modules per package. Hopefully a small amount of testing could confirm whether we run into anything, but there's probably some edge case we'll hit later than we'd want to. Ultimately that would just be a further bug to fix. This is definitely a change I completely support.

@EPTamminga
Copy link
Contributor

Has anybody created an XSD for the DNN manifest at any point in time?

@bdukes
Copy link
Contributor

bdukes commented Jul 9, 2019

@EPTamminga there was one made by the community about ten years ago, but I don't know if there's any way to retrieve it. I had used it for intellisense in Visual Studio way back when.

It's referenced in this forum post: https://www.dnnsoftware.com/forums/forumid/160/threadid/317745/scope/posts, saying it was uploaded to a Gemini issue. I thought we had an archive of Gemini at some URL, but haven't been able to find it

@EPTamminga
Copy link
Contributor

EPTamminga commented Jul 9, 2019

@bdukes. Ok, time for a new try and polis up my experience in xsd's.

@valadas
Copy link
Contributor

valadas commented Jul 10, 2019

I have spent quite some time trying to build one, the problem is that the package type is a prop and what it can contain varies depending on that prop. If it was a node, then an xsd would be awesome but I have found no way to alter the rules on childs based on a prop.

@valadas
Copy link
Contributor

valadas commented Jul 10, 2019

Also, the old xds are actually part of the platform repo and get on each dnn website too
https://github.com/dnnsoftware/Dnn.Platform/tree/development/Website/Components/ResourceInstaller

@EPTamminga
Copy link
Contributor

That is interesting, I see the problem.
The current XSD's in the repo solved this by using different XSD's based upon the extentsion type (module, them, provider).

@kestasjk
Copy link
Contributor Author

Thanks for the feedback guys. Just to let you know we're using this approach with an in-house module we're developing and it is working very nicely. I've got an active directory login module, SMS login module, several business specific modules, they're all MVC using JSON etc, arguments with paths, redirection, and working nicely and quickly.
The update process is very quick and straightforward, and we have all our modules in a single place in source control and all our business specific code/content in a single DesktopModules folder.

I will strip out the business specific stuff and upload the a multi-desktop-module example package to GitHub asap. I think any MVC developer would immediately understand this, so kudos whoever wrote the MVC support in.

Regarding things getting confused e.g. the edit menu: You can share the settings controller between multiple desktop modules or have an individual one per controller, if I run into any trouble I will let you know.

@valadas valadas added this to the 10.0.0 milestone Jul 29, 2019
@stale
Copy link

stale bot commented Oct 27, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Oct 27, 2019
@stale stale bot removed the stale label Oct 29, 2019
Copy link
Contributor

@david-poindexter david-poindexter left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks good to me

@valadas valadas merged commit 60b916a into dnnsoftware:development Oct 29, 2019
DnnSoftwarePersian pushed a commit to DnnSoftwarePersian/Dnn.Platform that referenced this pull request May 17, 2020
…on set by the install manifest, instead of using the Package FriendlyName and Description, allowing multiple DesktopModules per Package. (dnnsoftware#2887)
bdukes pushed a commit to bdukes/Dnn.Platform that referenced this pull request Sep 3, 2021
set by the install manifest, instead of using the Package FriendlyName and
Description, allowing multiple DesktopModules per Package. (dnnsoftware#2887)
bdukes pushed a commit to bdukes/Dnn.Platform that referenced this pull request Sep 3, 2021
By allowing a DesktopModule to have an explicit FriendlyName and
Description set by the install manifest, instead of using the Package
FriendlyName and Description, allows multiple DesktopModules per Package

Fixes dnnsoftware#2887
Reimplementation of dnnsoftware#2887 and dnnsoftware#2886
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants