Removed NETSTANDARD2_0 IFDefs and added Brackets #451

KyleJFischer · 2020-05-22T22:26:48Z

Removed NETSTANDARD2_) IfDefs to allow later versions of .net to be able to use the external execution feature.

Also added { because it won't let me build if I didn't fix them. Can remove them if we want it the old way.

… me build if I didn't fix them

k8s-ci-robot · 2020-05-22T22:26:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: KyleJFischer
To complete the pull request process, please assign krabhishek8260
You can assign the PR to them by writing /assign @krabhishek8260 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2020-05-22T22:26:51Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2020-05-22T22:26:56Z

Welcome @KyleJFischer!

It looks like this is your first PR to kubernetes-client/csharp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/csharp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

KyleJFischer · 2020-05-22T23:02:20Z

@k8s-ci-robot /check-cla

KyleJFischer · 2020-05-22T23:08:35Z

/retest

k8s-ci-robot · 2020-05-22T23:08:48Z

@KyleJFischer: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

KyleJFischer · 2020-05-22T23:11:06Z

/check-cla

KyleJFischer · 2020-05-22T23:13:19Z

/assign @krabhishek8260

…it is compatiable with 4.5.2

…tVariables

KyleJFischer · 2020-05-23T00:08:05Z

Sorry about all the extra commits, first time contributing. Let me know if there is anything else you need me to change!

NasAmin · 2020-05-26T09:04:37Z

src/KubernetesClient/KubernetesClientConfiguration.ConfigFile.cs

@@ -411,7 +407,6 @@ public static string RenewAzureToken(string tenantId, string clientId, string ap
            throw new KubeConfigException("Refresh not supported.");
        }

-#if NETSTANDARD2_0


Not sure what impact this will have in terms for backwards compatibility.
Will this cause any harm if this block of code runs for non .Net core clients?

I'll try running some more testing on this in a bit. If others could test different setups as well, that would probably be the most ideal.

tg123 · 2020-05-26T22:31:11Z

I think it is OK to remove this build condition.

add in #359
ping @talboren for more info

ghost · 2020-05-27T06:52:18Z

I think it is OK to remove this build condition.

add in #359
ping @talboren for more info

At the time I had several problems with these tests that required the preprocessor directives, not quite sure what will be the impact of removing these but it seems that build passes.

@tg123 you mentioned before that you're working on migrating the codebase to .netstandard2.0, how's that going? anything we can help with?

btw, not sure it has something to do with this specific pull request but @KyleJFischer please bare in mind the following issue: #396 (although I haven't suffered from the problems mentioned in there)

tg123 · 2020-05-27T12:40:45Z

src/KubernetesClient/KubernetesClientConfiguration.ConfigFile.cs

            if (config.EnvironmentVariables != null)
            {
                foreach (var configEnvironmentVariableKey in config.EnvironmentVariables.Keys)
                {
-                    process.StartInfo.Environment.Add(key: configEnvironmentVariableKey,
+                    process.StartInfo.EnvironmentVariables.Add(key: configEnvironmentVariableKey,


I checked the doc
EnvironmentVariables === Environment

just curious why this was changed

If you are asking why I changed it, I had read above the target .net framework for this client was 4.5.2 which unfortunately doesn't have the Environment. However, EnvironmentVariables does exist in 4.5.2 and is in the latest preview of .net 5. Source

If you are asking why it changed in general, well, I can't help you there :)

Hopefully that clears up any confusion.

tg123 · 2020-05-27T12:41:22Z

/LGTM

already moved to netstandard2
thanks @talboren

NasAmin · 2020-05-28T09:46:30Z

Is there anything else specific this PR is waiting on? I'd really appreciate if it can be merged please.
@KyleJFischer I have tried your PR changes locally to pull deployments and namespaces from multiple clusters on .NET Core 3.1 project. It works perfectly.

brendandburns · 2020-05-29T18:32:42Z

/approve

Removed NETSTANDARD2_0 IFDefs and Fixed some brackets it wouldn't let…

8d5f331

… me build if I didn't fix them

k8s-ci-robot requested review from brendandburns and krabhishek8260 May 22, 2020 22:26

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 22, 2020

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 22, 2020

Removed bracket changes

aa7ae64

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 22, 2020

Fixed Formatting hopefully

ae9ffee

k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 22, 2020

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 22, 2020

k8s-ci-robot assigned krabhishek8260 May 22, 2020

Updated StartInfo.EnvironmentVariables from StartInfo.Environment so …

afc42f6

…it is compatiable with 4.5.2

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 22, 2020

Kyle J Fischer added 2 commits May 22, 2020 19:35

Forgot I had changed this and didn't check git diff till now. My Bad

4dbabca

Missed a spot where it was referencing Envrionment and not Environmen…

9f9bfed

…tVariables

KyleJFischer mentioned this pull request May 23, 2020

#if NETSTANDARD2_0 Preprocessor tags in KubernetesClientConfig #450

Closed

NasAmin approved these changes May 26, 2020

View reviewed changes

NasAmin reviewed May 26, 2020

View reviewed changes

tg123 reviewed May 27, 2020

View reviewed changes

k8s-ci-robot assigned tg123 May 27, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 27, 2020

NasAmin approved these changes May 28, 2020

View reviewed changes

NasAmin approved these changes May 29, 2020

View reviewed changes

brendandburns merged commit 187ec8f into kubernetes-client:master May 29, 2020

Removed NETSTANDARD2_0 IFDefs and added Brackets #451

Removed NETSTANDARD2_0 IFDefs and added Brackets #451

Uh oh!

Conversation

KyleJFischer commented May 22, 2020

Uh oh!

k8s-ci-robot commented May 22, 2020

Uh oh!

k8s-ci-robot commented May 22, 2020

Uh oh!

k8s-ci-robot commented May 22, 2020

Uh oh!

KyleJFischer commented May 22, 2020

Uh oh!

KyleJFischer commented May 22, 2020

Uh oh!

k8s-ci-robot commented May 22, 2020

Uh oh!

KyleJFischer commented May 22, 2020

Uh oh!

KyleJFischer commented May 22, 2020

Uh oh!

KyleJFischer commented May 23, 2020

Uh oh!

NasAmin May 26, 2020

Choose a reason for hiding this comment

Uh oh!

KyleJFischer May 26, 2020

Choose a reason for hiding this comment

Uh oh!

tg123 commented May 26, 2020

Uh oh!

ghost commented May 27, 2020

Uh oh!

tg123 May 27, 2020

Choose a reason for hiding this comment

Uh oh!

KyleJFischer May 27, 2020

Choose a reason for hiding this comment

Uh oh!

tg123 commented May 27, 2020

Uh oh!

NasAmin commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brendandburns commented May 29, 2020

Uh oh!

Uh oh!

NasAmin commented May 28, 2020 •

edited

Loading