Migrates to net core #114
Open
Migrates to net core #114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The main issues were:
|
AdamJamesNaylor
commented
Jan 28, 2018
| @@ -17,10 +17,10 @@ namespace NAppUpdate.Framework.Utils | |||
| /// Starts the cold update process by extracting the updater app from the library's resources, | |||
| /// passing it all the data it needs and terminating the current application | |||
| /// </summary> | |||
| internal static class NauIpc | |||
| public static class NauIpc | |||
There was a problem hiding this comment.
I had to elevate the access here so that the tests would build. Not sure how internal was possible before?
AdamJamesNaylor
commented
Jan 28, 2018
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Text; | ||
|
|
||
| namespace NAppUpdate.Tests.Conditions | ||
| { | ||
| using Xunit; |
There was a problem hiding this comment.
The only real change to the test project was moving it across to XUnit. I have no preference on net core test frameworks but I know XUnit works.
AdamJamesNaylor
commented
Jan 28, 2018
| { | ||
| var rules = Directory.GetAccessControl(path).GetAccessRules(true, true, typeof(SecurityIdentifier)); | ||
| public static bool HaveWritePermissionsForFileOrFolder(string path) { | ||
| var rules = new FileSecurity(path, AccessControlSections.All).GetAccessRules(true, true, typeof(SecurityIdentifier)); |
There was a problem hiding this comment.
This functionality currently isn't covered by tests (I believe?), happy to fill this gap if needed.
AdamJamesNaylor
commented
Jan 28, 2018
| <Compile Include="Utils\PermissionsCheck.cs" /> | ||
| <Compile Include="Utils\ProcessStartFailedException.cs" /> | ||
| <Compile Include="Utils\Reflection.cs" /> | ||
| <PackageReference Include="Microsoft.Windows.Compatibility" Version="2.0.0-preview1-25914-04" /> |
There was a problem hiding this comment.
If you didn't want this dependency you could consider making the RegistryTask more 'optional'.
|
Thank you @AdamJamesNaylor for you work! can you please ping me when this is ready for review and testing? Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.