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.

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

Make CA certificate optional in config #105

Merged
merged 1 commit into from
Aug 28, 2019
Merged

Make CA certificate optional in config #105

merged 1 commit into from
Aug 28, 2019

Conversation

seleznev
Copy link
Collaborator

CA certificate is not required by Kubernetes client actually: https://github.com/kubernetes-client/python/blob/v10.0.1/kubernetes/client/rest.py#L72-L77.

@seleznev seleznev added the bug Something isn't working label Aug 27, 2019
@codecov-io
Copy link

codecov-io commented Aug 27, 2019 •
edited
Loading

Codecov Report

Merging #105 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #105      +/-   ##
==========================================
+ Coverage   82.13%   82.14%   +0.01%     
==========================================
  Files          16       16              
  Lines        1377     1378       +1     
==========================================
+ Hits         1131     1132       +1     
  Misses        246      246
Impacted Files Coverage Δ
k8s_handle/config.py 100% <100%> (ø) ⬆️

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 733104f...486369f. Read the comment docs.

@dekhtyarev
Copy link
Collaborator

В Readme укажем об опциональности параметра?

@seleznev
Copy link
Collaborator Author

В Readme укажем об опциональности параметра?

Что-то я не вижу где там написано, что он прям обязательно нужен. Есть просто упоминания... Мы точно хотим акцентировать на этом внимание? 🤔

@dekhtyarev
Copy link
Collaborator

В Readme укажем об опциональности параметра?

Что-то я не вижу где там написано, что он прям обязательно нужен. Есть просто упоминания... Мы точно хотим акцентировать на этом внимание?

@seleznev
Copy link
Collaborator Author

Кажется вот тут что-то есть

Строго говоря, тут про конкретный пример деплоя, а там она явно используется: https://github.com/2gis/k8s-handle-example/blob/without-kubeconfig/config.yaml#L5. Впрочем, пустое значение тоже работает, поэтому ок, добавил пометку.

А как обыграть эту часть - пока не представляю. Можно оставить

Там же выше "Create kubeconfig(~/.kube/config) or skip if you already have one". 👼

dekhtyarev
dekhtyarev previously approved these changes Aug 27, 2019
View reviewed changes
furiousassault
furiousassault previously approved these changes Aug 27, 2019
View reviewed changes
Copy link
Collaborator

@furiousassault furiousassault left a comment

Choose a reason for hiding this comment

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

За исключением минорных комментов, LGTM.

k8s_handle/config.py Show resolved Hide resolved
k8s_handle/config.py Show resolved Hide resolved
@furiousassault furiousassault self-requested a review August 27, 2019 12:14
@dekhtyarev dekhtyarev merged commit df2667b into master Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dekhtyarev dekhtyarev dekhtyarev approved these changes

@furiousassault furiousassault furiousassault approved these changes

@dcleric dcleric Awaiting requested review from dcleric

@rvadim rvadim Awaiting requested review from rvadim

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@seleznev @codecov-io @dekhtyarev @furiousassault