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.

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

support default HostIp #1791

Merged
merged 1 commit into from
Oct 24, 2022
Merged

Conversation

Stevent-fei
Copy link
Collaborator

@Stevent-fei Stevent-fei commented Oct 21, 2022

Describe what this PR does / why we need it

#1787

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@github-actions github-actions bot added the test label Oct 21, 2022
@Stevent-fei Stevent-fei force-pushed the support_default_HostIP branch 2 times, most recently from 22a0e9d to 797cc3e Compare October 21, 2022 13:08
@Stevent-fei Stevent-fei linked an issue Oct 21, 2022 that may be closed by this pull request
if d.clusterEnv["RegistryDomain"] == nil {
d.clusterEnv["RegistryDomain"] = "sea.hub"
}
if d.clusterEnv["RegistryPort"] == nil {
Copy link
Member

Choose a reason for hiding this comment

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

use if ok to check its map key, and add more code comments.

Copy link
Collaborator

@VinceCui VinceCui left a comment

Choose a reason for hiding this comment

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

Attention UT.

}

func (d *SSHInfraDriver) GetClusterEnv() map[string]interface{} {
return d.clusterRenderData
if _, ok := d.clusterEnv["RegistryDomain"]; !ok {
d.clusterEnv["RegistryDomain"] = "sea.hub"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strings like "sea.hub", "5000", should be consts and put into well known common file.

@@ -141,11 +142,21 @@ func (d *SSHInfraDriver) GetHostIPListByRole(role string) []net.IP {
}

func (d *SSHInfraDriver) GetHostEnv(host net.IP) map[string]interface{} {
return d.hostEnvMap[host.String()]
hostEnv := d.hostEnvMap[host.String()]
if _, ok := hostEnv["HostIP"]; !ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keys like "HostIP" should be a const and put it into well known common file

@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2022

Codecov Report

Base: 20.66% // Head: 20.81% // Increases project coverage by +0.14% 🎉

Coverage data is based on head (9bfb8b6) compared to base (1cf55c3).
Patch coverage: 77.27% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1791      +/-   ##
==========================================
+ Coverage   20.66%   20.81%   +0.14%     
==========================================
  Files          71       71              
  Lines        6542     6558      +16     
==========================================
+ Hits         1352     1365      +13     
- Misses       5009     5012       +3     
  Partials      181      181              
Impacted Files Coverage Δ
pkg/cluster-runtime/installer.go 0.00% <0.00%> (ø)
pkg/cluster-runtime/scale.go 0.00% <0.00%> (ø)
pkg/infradriver/ssh_infradriver.go 38.83% <100.00%> (+4.11%) ⬆️

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.

@Stevent-fei Stevent-fei force-pushed the support_default_HostIP branch 2 times, most recently from ead06f2 to 9bfb8b6 Compare October 24, 2022 02:12
reference and set constants;

add comments
@Stevent-fei Stevent-fei force-pushed the support_default_HostIP branch from 9bfb8b6 to d1db1f3 Compare October 24, 2022 02:28
Copy link
Member

@kakaZhou719 kakaZhou719 left a comment

Choose a reason for hiding this comment

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

LGTM

@kakaZhou719 kakaZhou719 merged commit f05c753 into sealerio:main Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sealer support builtin env list
4 participants