Skip to content

Fix bugs in programmatic cluster config. Add unit test. #368

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 19, 2019
Merged

Fix bugs in programmatic cluster config. Add unit test. #368

merged 1 commit into from
Nov 19, 2019

Conversation

brendandburns
Copy link
Contributor

Fixes #365

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns

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

The pull request process is described here

Needs approval from an approver in each of these files:

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 k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 15, 2019
@brendandburns brendandburns requested review from drubin and removed request for itowlson and mbohlool November 15, 2019 05:44
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 15, 2019
drubin
drubin reviewed Nov 15, 2019
View reviewed changes
src/config.ts
@@ -233,6 +236,9 @@ export class KubeConfig {
}

public addUser(user: User) {
if (!this.users) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be better to put this in the constructor? I think it would be more logical and easier to follow too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker just curious before I add my lgtm (and it merges)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fields involved are public, so it's always possible that someone will overwrite them with something undefined or null, so the check here is safer.

@brendandburns
Copy link
Contributor Author

Comment addressed, please re-check.

Thanks!

@drubin
Copy link
Contributor

drubin commented Nov 19, 2019

/lgtm
Thanks for the clarification

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit 34cfa68 into kubernetes-client:master Nov 19, 2019
@brendandburns brendandburns deleted the configit branch July 8, 2020 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drubin drubin drubin left review comments

Assignees

@drubin drubin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Calling addCluster/User/Context fails if nothing was previously loaded
3 participants
@brendandburns @k8s-ci-robot @drubin