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

Target .Net Standard 2.0 #33

Merged
merged 1 commit into from
Aug 2, 2019

Conversation

bergmeister
Copy link
Contributor

@bergmeister bergmeister commented Jul 6, 2019

API's are nearly the same in terms of behaviour, the only lost functionality to cancel the async reading of the file.
This also highly reduced the size of the module from around 160 MB to 10MB
In practice one could even look at it in detail and exclude most DLLs (as PowerShell provides most of them therefore only DLLs from the project itself and it's NuGet packages are technically needed.

@codecov
Copy link

codecov bot commented Jul 6, 2019

Codecov Report

Merging #33 into master will decrease coverage by 0.12%.
The diff coverage is 9.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #33      +/-   ##
==========================================
- Coverage   47.86%   47.73%   -0.13%     
==========================================
  Files          26       26              
  Lines         796      796              
==========================================
- Hits          381      380       -1     
- Misses        415      416       +1
Impacted Files Coverage Δ
src/Cmdlets/UseKubeContextCmdlet.cs 0% <0%> (ø) ⬆️
src/Cmdlets/CompareKubeResourceCmdlet.cs 0% <0%> (ø) ⬆️
src/KubeYamlDeserializer.cs 0% <0%> (ø) ⬆️
src/SetKubeConfigCmdlet.cs 0% <0%> (ø) ⬆️
src/KubeResourceComparer.cs 53.94% <0%> (ø) ⬆️
src/Cmdlets/UpdateKubeResourceCmdlet.cs 70% <33.33%> (-2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05a9a3c...c6d9a92. Read the comment docs.

Copy link
Owner

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Is there anything left TODO here? Since this is a draft PR

@bergmeister
Copy link
Contributor Author

bergmeister commented Aug 2, 2019

Thanks. No, there shouldn't be anything left to do. I created it only as draft because I thought you might want me to make some changes after first review. I've marked my other 2 PRs as ready for review now as well.

@bergmeister bergmeister marked this pull request as ready for review August 2, 2019 17:14
@felixfbecker felixfbecker merged commit ea51307 into felixfbecker:master Aug 2, 2019
@felixfbecker
Copy link
Owner

Thanks, awesome size reduction!

@felixfbecker
Copy link
Owner

Hmm, it seems like newer versions of the PowerShell SDK are not compatible with .netstandard but only .netcore? https://travis-ci.org/felixfbecker/PSKubectl/builds/567063841#L745

@bergmeister
Copy link
Contributor Author

I would not update the PS SDK until 6.1 is deprecated. You don't need to reference 6.2 for it to work on 6.2. One would only need to reference it if you use APIs that are only available in newer versions. I can take a look at the issue briefly but I think you might be able to dump the SDK and replace it with SMA but I cannot say that for sure until I've looked at what APIs you use

@felixfbecker
Copy link
Owner

Ah, makes sense.
What's SMA?

@bergmeister
Copy link
Contributor Author

System.Management.Automation
This is just a reference library (a bit like a header file as a C++ analogy) so that you know which APIs PowerShell exposes, one needs the PS SDK only if you wanted to e.g. host PowerShell yourself but this is not needed for a cmdlet because PowerShell already hosts you as a guest.
It works on my branch already but the module building logic would need to be tweaked to avoid publishing the whole runtime again similar to how it was before we improved it to use netstandard, unfortunately NuGet has a big which makes dotnet build not work to just get the necessary dlls because all we need is your kubectl dll and the ones from the nuget packages except the PowerShell onew because the PowerShell runtime already has them

@felixfbecker
Copy link
Owner

Would it help to add some commands to build.ps1? E.g. delete unneeded DLLs

@bergmeister
Copy link
Contributor Author

bergmeister commented Aug 2, 2019

Usually one just takes the output from dotnet build but due to this Nuget issue, we don't get the DLLs from the other NuGet packages. Deleting is not straightforward (e.g. just deleting System.* and Microsoft.* also removes some required DLLs such as e.g. Microsoft.Extensions.Logging.Abstractions). With different .Net SDKs, the list of files to be deleted will change over time, therefore the approach is usually to white-list the ones you want but this comes with the drawback of having to maintain/update that list if one updates e.g. NuGet packages and in a newer version, they added a new DLL for example... We could reduce the module size furthermore from something like 10 MB to around 1-2 MB. Would you be interested in that?

@felixfbecker
Copy link
Owner

Hmm I don't know if it's worth the maintenance burden. Is module size really a concern?

@bergmeister
Copy link
Contributor Author

module size means slower installation and generally just lots of space that accumulates. We optimised already 90MB away with the .Net Standard2.0 tweak, so one could argue that we should follow the 80-20 rule and stop here since the additional improvement of is much smaller now. I'll try to see if there is a way using dotnet build by avoiding the Nuget bug because then there would not be the maintenance burden...

@felixfbecker
Copy link
Owner

🎉 This PR is included in version 0.11.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants