Install and use licenses in standard file locations #741

timbussmann · 2017-08-15T12:43:13Z

ServiceInsight will now install licenses in the following location:

%localappdata%\ParticularSoftware\license.xml

...and it will use licenses located in any of the following locations:

{Application base directory\license.xml} (e.g. C:\MyApps\MyEndpoint\license.xml)
%programdata%\ParticularSoftware\license.xml
%localappdata%\ParticularSoftware\license.xml

timbussmann · 2017-08-17T09:24:38Z

this is ready for review. Please note this is only a suggestion how to deal with the changes to the new license locations. Quickly tested the behavior locally.

adamralph

Aside from the suppressions, the changes LGTM.

adamralph · 2017-08-18T11:11:37Z

src/ServiceInsight/GlobalSuppressions.cs

+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.ReadabilityRules", "SA1128:ConstructorInitializerMustBeOnOwnLine", Justification = "Generated File.")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1516:ElementsMustBeSeparatedByBlankLine", Justification = "Generated File.")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.Naming Rules", "SA1303:ConstFieldNamesMustBeginWithUpperCaseLetter", Justification = "Generated File.")]


These lines have been added to suppress the StyleCop violations found in the source files added by the new licensing package.

We should not be doing this here because this will suppress those rules for all files.

I'm not sure if StyleCop.Analyzers has a way to suppress specific files from analysis (the best I could find was this issue which I commented on way back), but I have raised https://github.com/Particular/Operations.Licensing/pull/37 to fix these violations in the source package, so if that PR is merged then we can update to the latest package and remove these lines.

FYI https://github.com/Particular/Operations.Licensing/pull/37 has been merged.

@Particular/operations-licensing-maintainers please let us know when a new alpha has been released so we can update to it in this PR.

adamralph · 2017-08-18T11:12:38Z

src/ServiceInsight/GlobalSuppressions.cs

+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.LayoutRules", "SA1516:ElementsMustBeSeparatedByBlankLine", Justification = "Generated File.")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.Naming Rules", "SA1303:ConstFieldNamesMustBeginWithUpperCaseLetter", Justification = "Generated File.")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Code", "PCR0001:Await used without specifying ConfigureAwait", Justification = "Default in WPF is fine.")]


This line is a repitition of a line above and should be removed.

HEskandari · 2017-08-22T04:27:18Z

src/ServiceInsight/App_Packages/Particular.Licensing/License.cs

@@ -41,7 +41,7 @@ public License()

        public bool ValidForApplication(string applicationName)
        {
-            return ValidApplications.Contains(applicationName) || ValidApplications.Contains("All");
+            return ValidApplications.Contains(applicationName) || ValidApplications.Contains("All") || applicationName == "NServiceBus";


Do we need "NServiceBus" for the app name here?

It was added to accommodate for older licenses so I think we can just leave as is unless its causing issues for SI?

See:

https://github.com/Particular/Operations.Licensing/pull/28

Discussed in @Particular/serviceinsight-maintainers sync today. We're OK with this. It doesn't affect SI.

andreasohlund · 2017-08-22T11:18:32Z

1.0.0-alpha0007 is now on myget https://www.myget.org/feed/particular/package/nuget/Particular.Licensing.Sources/1.0.0-alpha0007

…

On Tue, Aug 22, 2017 at 9:15 AM Adam Ralph ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ServiceInsight/App_Packages/Particular.Licensing/License.cs <#741 (comment)> : > @@ -41,7 +41,7 @@ public License() public bool ValidForApplication(string applicationName) { - return ValidApplications.Contains(applicationName) || ValidApplications.Contains("All"); + return ValidApplications.Contains(applicationName) || ValidApplications.Contains("All") || applicationName == "NServiceBus"; Discussed in @Particular/serviceinsight-maintainers <https://github.com/orgs/Particular/teams/serviceinsight-maintainers> sync today. We're OK with this. It doesn't affect SI. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#741 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHoZH7YBzZT2u70LRW7LIi9M9nPqL7kks5saoAHgaJpZM4O3ifB> .

timbussmann · 2017-08-22T12:00:30Z

updated to alpha7

adamralph

Thanks @timbussmann, the changes look good to me.

Can you please:

Entirely revert the changes to GlobalSuppressions.cs. There's still an EOL change in there which causes the file to be shown in the diff.
Squash all the commits into one

Then I'll be happy to approve/merge.

timbussmann · 2017-08-22T18:53:10Z

@adamralph reverted changes to the globalsuppressions.cs. Can't you squash yourself when merging using github's squash & merge feature?

adamralph · 2017-08-23T12:44:11Z

Thanks @timbussmann.

Can't you squash yourself when merging using github's squash & merge feature?

We never use that, since it does an ff merge and looks like a push straight to master in the history. I'd use it if it had a no-ff option.

I've squash locally and force pushed.

adamralph

@Particular/serviceinsight-maintainers I've squashed @timbussmann's commits. If you're happy with the result can you please merge.

adamralph · 2017-08-24T10:36:07Z

@timbussmann is there any functional change for the user here? Does this mean SI now respects new locations for license files?

timbussmann · 2017-08-25T06:11:36Z

@adamralph yes, SI would now search the new file locations for licenses and would also install new licenses to the new storage location (to the user local app data) additionally to the registry.

adamralph · 2017-08-25T11:14:55Z

Thanks @timbussmann, I've updated the title and description, and marked this as a feature. Can you please check to see if the title and description are correct?

@Particular/serviceinsight-maintainers given this is a feature, I've changed the next milestone from 1.7.2 to 1.8.0.

timbussmann · 2017-08-25T12:26:00Z

@adamralph LGTM except that the file paths are alawys using particularsoftware, nut just particular (also, they are lowercase)

adamralph · 2017-08-25T12:37:31Z

@timbussmann

the file paths are alawys using particularsoftware, nut just particular

👍 thanks, corrected.

also, they are lowercase

I see PascalCase here:

ServiceInsight/src/ServiceInsight/App_Packages/Particular.Licensing/FilePathLicenseStore.cs

Line 10 in 249c565

    
           public static readonly string UserLevelLicenseLocation = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData), "ParticularSoftware", "license.xml");

timbussmann · 2017-08-25T12:44:32Z

@adamralph ah you're right. Sorry it was just the file name applied the lowercasing rule. Wondering whether that should be changed for the rest of the path as well now :/

adamralph self-requested a review August 15, 2017 12:46

timbussmann changed the title ~~[WIP] Update licensing~~ Update licensing Aug 17, 2017

adamralph requested review from a team and adamralph and removed request for adamralph August 17, 2017 11:31

adamralph suggested changes Aug 18, 2017

View reviewed changes

HEskandari reviewed Aug 22, 2017

View reviewed changes

adamralph suggested changes Aug 22, 2017

View reviewed changes

timbussmann force-pushed the update-licensing branch from 49dc42f to 571150e Compare August 22, 2017 18:50

update licensing package

f340ac0

adamralph force-pushed the update-licensing branch from 250930a to f340ac0 Compare August 23, 2017 12:43

adamralph approved these changes Aug 23, 2017

View reviewed changes

WilliamBZA merged commit 249c565 into master Aug 24, 2017

WilliamBZA deleted the update-licensing branch August 24, 2017 06:32

adamralph added this to the 1.7.3 milestone Aug 24, 2017

adamralph changed the title ~~Update licensing~~ Read and store licenses in standard file locations Aug 25, 2017

adamralph changed the title ~~Read and store licenses in standard file locations~~ Use and install licenses in standard file locations Aug 25, 2017

adamralph added the Feature label Aug 25, 2017

adamralph changed the title ~~Use and install licenses in standard file locations~~ Install and use licenses in standard file locations Aug 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install and use licenses in standard file locations #741

Install and use licenses in standard file locations #741

timbussmann commented Aug 15, 2017 •

edited by adamralph

Loading

timbussmann commented Aug 17, 2017

adamralph left a comment

adamralph Aug 18, 2017

adamralph Aug 18, 2017

adamralph Aug 18, 2017

HEskandari Aug 22, 2017

andreasohlund Aug 22, 2017

adamralph Aug 22, 2017

andreasohlund commented Aug 22, 2017 via email

timbussmann commented Aug 22, 2017

adamralph left a comment

timbussmann commented Aug 22, 2017

adamralph commented Aug 23, 2017

adamralph left a comment

adamralph commented Aug 24, 2017

timbussmann commented Aug 25, 2017

adamralph commented Aug 25, 2017

timbussmann commented Aug 25, 2017

adamralph commented Aug 25, 2017

timbussmann commented Aug 25, 2017

Install and use licenses in standard file locations #741

Install and use licenses in standard file locations #741

Conversation

timbussmann commented Aug 15, 2017 • edited by adamralph Loading

timbussmann commented Aug 17, 2017

adamralph left a comment

Choose a reason for hiding this comment

adamralph Aug 18, 2017

Choose a reason for hiding this comment

adamralph Aug 18, 2017

Choose a reason for hiding this comment

adamralph Aug 18, 2017

Choose a reason for hiding this comment

HEskandari Aug 22, 2017

Choose a reason for hiding this comment

andreasohlund Aug 22, 2017

Choose a reason for hiding this comment

adamralph Aug 22, 2017

Choose a reason for hiding this comment

andreasohlund commented Aug 22, 2017 via email

timbussmann commented Aug 22, 2017

adamralph left a comment

Choose a reason for hiding this comment

timbussmann commented Aug 22, 2017

adamralph commented Aug 23, 2017

adamralph left a comment

Choose a reason for hiding this comment

adamralph commented Aug 24, 2017

timbussmann commented Aug 25, 2017

adamralph commented Aug 25, 2017

timbussmann commented Aug 25, 2017

adamralph commented Aug 25, 2017

timbussmann commented Aug 25, 2017

timbussmann commented Aug 15, 2017 •

edited by adamralph

Loading