Skip to content
This repository has been archived by the owner on Mar 13, 2022. It is now read-only.

config: Fix persist_config flag and function calls #169

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

jfrabaute
Copy link
Contributor

The persist_config flag was setting the kwargs['config_persister'] to
the result of the function kcfg.save_changes and not the function
kcfg.save_changes itself.

Once this is fixed, the second problem was that the function was called
with an argument when it's defined without argument so an exception was
raised.

@k8s-ci-robot
Copy link
Contributor

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.


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 k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 4, 2019
@codecov-io
Copy link

codecov-io commented Oct 5, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@7ea5cb4). Click here to learn what that means.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage          ?   92.52%           
=========================================
  Files             ?       13           
  Lines             ?     1485           
  Branches          ?        0           
=========================================
  Hits              ?     1374           
  Misses            ?      111           
  Partials          ?        0
Impacted Files Coverage Δ
config/kube_config_test.py 95.66% <100%> (ø)
config/kube_config.py 84.25% <25%> (ø)

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 7ea5cb4...64662bb. Read the comment docs.

@yliaog
Copy link
Contributor

yliaog commented Oct 8, 2019

please sign cla
please add a test to verify the fix

@jfrabaute
Copy link
Contributor Author

CLA signed

@k8s-ci-robot 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 Oct 23, 2019
@jfrabaute
Copy link
Contributor Author

Hi @yliaog

Regarding the test, from what I understand from the code, the problem is in the function "get_kube_config_loader_for_yaml_file" (this is where the fix is).
Should I add a test for this function? I'm asking because as it has a "
" it might just be a private function that should not be tested. The problem is that I can't find another way to test it as calling, as calling other functions using this one does not allow me to check that _config_persister is set correctly in the KubeConfigLoader object.
So, is that ok to add a test for "_get_kube_config_loader_for_yaml_file"?

Thanks.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 14, 2020
@jfrabaute
Copy link
Contributor Author

I've added 2 tests for the "_get_kube_config_loader_for_yaml_file" function.
Keep me posted if that's correct or not.

Thanks.

The persist_config flag was setting the kwargs['config_persister'] to
the result of the function kcfg.save_changes and not the function
kcfg.save_changes itself.

Once this is fixed, the second problem was that the function was called
with an argument when it's defined without argument so an exception was
raised.
@jfrabaute
Copy link
Contributor Author

Hi

Any news about reviewing this PR?

Thanks.

@roycaihw
Copy link
Member

/kind bug
/lgtm
/approve

Thanks for sending the fix!

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. labels Feb 21, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jfrabaute, roycaihw

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2020
@k8s-ci-robot k8s-ci-robot merged commit ff9a5f3 into kubernetes-client:master Feb 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants