Skip to content

refactor: adjust the order of merging kubeadm configs #2567

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

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

fengxsong
Copy link
Collaborator

@fengxsong fengxsong commented Feb 13, 2023

Signed-off-by: fengxsong [email protected]

Seems it's already implemented but needs some adjustments. #2562

@codecov
Copy link

codecov bot commented Feb 13, 2023

Codecov Report

Base: 65.09% // Head: 65.09% // No change to project coverage 👍

Coverage data is based on head (43355aa) compared to base (c89f7e1).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2567   +/-   ##
=======================================
  Coverage   65.09%   65.09%           
=======================================
  Files           8        8           
  Lines         573      573           
=======================================
  Hits          373      373           
  Misses        168      168           
  Partials       32       32           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fengxsong fengxsong force-pushed the feat_2562 branch 2 times, most recently from 76e2324 to 40b3bc2 Compare February 13, 2023 07:20
k.ImageKubeVersion = k.getKubeVersionFromImage()
for _, fn := range []string{
"", // generate default kubeadm configs
k.getDefaultKubeadmConfig(), // merging from predefined path of file if file exists
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. k.getDefaultKubeadmConfig()
  2. “”

is OK

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This way in the comment, as well as the original, both are setting default values first, then dst will not be overwritten even if the src is set. IMO, with mergo's transformers is more intuitive.

@cuisongliu
Copy link
Collaborator

@cuisongliu cuisongliu marked this pull request as draft February 13, 2023 10:17
@fengxsong fengxsong marked this pull request as ready for review February 13, 2023 11:02
@cuisongliu
Copy link
Collaborator

@cuisongliu cuisongliu merged commit 668f4bd into labring:main Feb 13, 2023
@fengxsong fengxsong deleted the feat_2562 branch February 14, 2023 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants