-
Notifications
You must be signed in to change notification settings - Fork 829
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: repair flaking test job of setup e2e test environment #3682
Conversation
Welcome @chaosi-zju! It looks like this is your first PR to karmada-io/karmada 🎉 |
Hi! @chaosi-zju Glad to see your contribution! Usually please use # an example
# in branch `fake-ci`
git fetch upstream
git rebase upstream/master
git commit -s --amend
git push -f |
Signed-off-by: chaosi-zju <chaosi@zju.edu.cn>
thank you very much~ |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## master #3682 +/- ##
==========================================
+ Coverage 56.63% 56.64% +0.01%
==========================================
Files 221 221
Lines 20838 20834 -4
==========================================
+ Hits 11801 11802 +1
+ Misses 8415 8409 -6
- Partials 622 623 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
Generally looks good to me.
@yike21 do you have any other comments?
rm -f "${main_config}" | ||
rm -f "${member_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.
It just feels a little bit rough to delete the entire kubeconfig file.
If the file contains a cluster not created by kind, the configuration will be removed unexpectedly.
It's not a big deal though.
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.
Generally LGTM. It seems that the logic for removing kubeconfig is included here. And the logic for removing kind clusters scope is expanded.
I think it's more suitable to just delete kind clusters which created by local-up-karmada.sh
.
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.
It just feels a little bit rough to delete the entire kubeconfig file. If the file contains a cluster not created by kind, the configuration will be removed unexpectedly.
It's not a big deal though.
Although kind delete
is capable of cleaning kubeconfig if specificed $KUBECONFIG
or --kubeconfig
, it is not as effective as expected. I have tried these two modification method:
- one
export KUBECONFIG="${main_config}":"${member_config}"
kind delete clusters --all >> "${log_file}" 2>&1
unset KUBECONFIG
- two
kind delete clusters --kubeconfig "${main_config}" karmada-host
kind delete clusters --kubeconfig "${member_config}" member1 member2 member3
All above methods can not clean up kubeconfig content thoroughly.
So, I currently have no more elegant way to clean up kubeconfig.
By the way, even if no rm -f xxx_config
in delete_all_clusters
, following the original logic, the script will eventually execute rm -f xxx_config
in util::create_cluster
.
the code of util::create_cluster
:
function util::create_cluster() {
local cluster_name=${1}
local kubeconfig=${2}
local kind_image=${3}
local log_path=${4}
local cluster_config=${5:-}
mkdir -p ${log_path}
rm -rf "${log_path}/${cluster_name}.log"
rm -f "${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.
OK. got it.
LGTM, thanks for your work! |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
What type of PR is this?
/kind flake
What this PR does / why we need it:
for CI stability
Which issue(s) this PR fixes:
Fixes #3667
Special notes for your reviewer:
@RainbowMango @XiShanYongYe-Chang @yike21
Does this PR introduce a user-facing change?: