felixfbecker / PSKubectl

kubectl with the power of the object pipeline
MIT License
61 stars 9 forks source link

Target .Net Standard 2.0 #33

Closed bergmeister closed 5 years ago

bergmeister commented 5 years ago

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[bot] commented 5 years ago

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%> (ø) :arrow_up:
src/Cmdlets/CompareKubeResourceCmdlet.cs 0% <0%> (ø) :arrow_up:
src/KubeYamlDeserializer.cs 0% <0%> (ø) :arrow_up:
src/SetKubeConfigCmdlet.cs 0% <0%> (ø) :arrow_up:
src/KubeResourceComparer.cs 53.94% <0%> (ø) :arrow_up:
src/Cmdlets/UpdateKubeResourceCmdlet.cs 70% <33.33%> (-2%) :arrow_down:

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.

bergmeister commented 5 years ago

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.

felixfbecker commented 5 years ago

Thanks, awesome size reduction!

felixfbecker commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Ah, makes sense. What's SMA?

bergmeister commented 5 years ago

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 commented 5 years ago

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

bergmeister commented 5 years ago

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 commented 5 years ago

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

bergmeister commented 5 years ago

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 commented 5 years ago

:tada: This PR is included in version 0.11.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: