-
Notifications
You must be signed in to change notification settings - Fork 53
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.
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
fix: remove check if the .kube/config exist and prevent replacing or reading it #1153
fix: remove check if the .kube/config exist and prevent replacing or reading it #1153
Conversation
Signed-off-by: Max batleforc <[email protected]>
Hi @batleforc. Thanks for your PR. I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1153 +/- ##
==========================================
+ Coverage 89.55% 89.69% +0.14%
==========================================
Files 415 415
Lines 42461 42461
Branches 2840 2843 +3
==========================================
+ Hits 38025 38085 +60
+ Misses 4409 4345 -64
- Partials 27 31 +4 ☔ View full report in Codecov by Sentry. |
@@ -89,7 +89,7 @@ export class KubeConfigApiService implements IKubeConfigApi { | |||
podName, | |||
namespace, | |||
containerName, | |||
['sh', '-c', `[ -f ${kubeConfigDirectory}/config ] || cat ${kubeConfigDirectory}/config`], | |||
['sh', '-c', `cat ${kubeConfigDirectory}/config`], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't understand why [ -f ${kubeConfigDirectory}/config ]
is not needed anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the [ -f ${kubeConfigDirectory}/config ]
is to do the next action if the file doesn't exist. The goal of this command is to check if the file exists, if it exists output it and merge it with the expected kubeconfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, if the file ${kubeConfigDirectory}/config
doesn't exist, then the exec command return not empty stdError. In this case we don't merge kubeConfig but creates it with the next exec command.
If so, can we do it in more clear way or at least add some comments explaining the logic please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some comment, is it enough or should I rework how i've done it ?
… kubeconfig Signed-off-by: Max batleforc <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akurinnoy, batleforc, tolusha The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build 3.16 :: dashboard_3.x/515: Console, Changes, Git Data |
Build 3.16 :: sync-to-downstream_3.x/7339: Console, Changes, Git Data |
Build 3.16 :: get-sources-rhpkg-container-build_3.x/7324: dashboard : 3.x :: Failed in 63042805 : BREW:BUILD/STATUS:UNKNOWN |
What does this PR do?
Fix an error made in #1141 (a copy and paste error), I forgot to remove the
[ -f ${kubeConfigDirectory}/config ]
that check if the file exists, and the goal here was either to replace theWhat issues does this PR fix or reference?
eclipse-che/che#22924
Is it tested? How?
Release Notes
N/A
Docs PR
N/A