add app installer to sealer run #1764
Conversation
Codecov ReportBase: 19.89% // Head: 19.77% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1764 +/- ##
==========================================
- Coverage 19.89% 19.77% -0.13%
==========================================
Files 69 69
Lines 6478 6524 +46
==========================================
+ Hits 1289 1290 +1
- Misses 5012 5057 +45
Partials 177 177
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. |
cmd/sealer/cmd/cluster/join.go
Outdated
| @@ -19,6 +19,9 @@ import ( | |||
| "io/ioutil" | |||
| "path/filepath" | |||
|
|
|||
| imagecommon "github.com/sealerio/sealer/pkg/define/options" | |||
There was a problem hiding this comment.
usually, there should be three groups separated by blank for go-imports
- the golang official package
- the package for the current project
- the others
pkg/cluster-runtime/installer.go
Outdated
| func (i *Installer) Install() error { | ||
| var ( | ||
| masters = i.infraDriver.GetHostIPListByRole(common.MASTER) | ||
| master0 = masters[0] |
There was a problem hiding this comment.
what if the length of masters is equal with zero?
There was a problem hiding this comment.
the length of masters will not equal to zero during the installation process, that's because we set default master role in the beginning which was got from user input.
pkg/cluster-runtime/installer.go
Outdated
| workers = getWorkerIPList(i.infraDriver) | ||
| all = append(masters, workers...) | ||
| image = i.infraDriver.GetClusterImageName() | ||
| launchCmds = i.infraDriver.GetClusterLaunchCmds() |
There was a problem hiding this comment.
if these codes not only for install cluster, it's prefer to use clusterLaunchCmds instead of launchCmds
| if err := i.runClusterHook(master0, PostInstallCluster); err != nil { | ||
| return nil, nil, err | ||
| } | ||
| func (i *Installer) installApp(master0 net.IP, cmds []string) error { |
There was a problem hiding this comment.
what's the reason for the master0 IP is needed here for install application
In fact, we should allow the application image to be deployed on either node
There was a problem hiding this comment.
agree with you, application image should to be deployed on any master node. here i just chose master0 to execute launch cmds. Do we have some scenarios that require this execute node?
pkg/cluster-runtime/installer.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("failed to install application: %s", err) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Where will the CMDS defined in the old image be executed?
There was a problem hiding this comment.
got it from image extension.Launch.Cmds
| if err != nil { | ||
| return nil, err | ||
| } | ||
| func NewImageMounter(imageEngine imageengine.Interface, hostsPlatform map[v1.Platform][]net.IP) (*ImagerMounter, error) { |
There was a problem hiding this comment.
nice job 👍
Make each method as atomized as possible, and we can use unit-test availability as a measure
| @@ -81,7 +82,8 @@ func NewInfraDriver(cluster *v2.Cluster) (InfraDriver, error) { | |||
| var err error | |||
| ret := &SSHInfraDriver{ | |||
| clusterName: cluster.Name, | |||
| clusterImagerName: cluster.Spec.Image, | |||
| clusterImageName: cluster.Spec.Image, | |||
There was a problem hiding this comment.
It's confusing here, what's the meaning of clusterImageName and will there be appImageName in the future?
There was a problem hiding this comment.
yes, its a string value that just represent the name of this clusterimage to launch.
2db3152 to
29820b4
Compare
add app installer to run process
29820b4 to
7f3ac7f
Compare
Describe what this PR does / why we need it
guesttoapplication.Does this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews