bugfix: fix apply #2116
bugfix: fix apply #2116
Conversation
starnop
force-pushed
the
fix-apply
branch
2 times, most recently
from
106accf to
9be5901
Compare
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #2116 +/- ##
==========================================
- Coverage 18.87% 12.51% -6.37%
==========================================
Files 95 258 +163
Lines 8899 22196 +13297
==========================================
+ Hits 1680 2778 +1098
- Misses 6991 19035 +12044
- Partials 228 383 +155
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 163 files with indirect coverage changes 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 in Codecov by Sentry. |
cmd/sealer/cmd/cluster/apply.go
Outdated
| @@ -127,12 +127,13 @@ func NewApplyCmd() *cobra.Command { | |||
| return applyClusterWithNew(cf, applyMode, imageEngine, imageSpec) | |||
| } | |||
|
|
|||
| if err := applyClusterWithExisted(cf, client, imageEngine, imageSpec); err != nil { | |||
| updated, err := applyClusterWithExisted(cf, client, imageEngine, imageSpec) | |||
There was a problem hiding this comment.
updated is too large to understand . Or i thought should we move those run app logic to applyClusterWithExisted.
There was a problem hiding this comment.
I have changed updated to clusterUpdated.
As with other comments, we should split run/apply to these steps:
- distribute rootfs
- apply/run cluster
- app/run application
And then, something just like clusterUpdated, skipPrepareAppMaterials will not be needed.
And we should do these code optimization ASAP.
| @@ -127,12 +127,13 @@ func NewApplyCmd() *cobra.Command { | |||
| return applyClusterWithNew(cf, applyMode, imageEngine, imageSpec) | |||
There was a problem hiding this comment.
applyClusterFileWithNew ? clusterfile contains app and cluster. so maybe it is better for us to use apply clusterfile ?
There was a problem hiding this comment.
in fact, it‘s just the part relevant to the cluster.
And as for what you said about the app, that's really unreasonable. And I have added a comment TODO: decouple the cluster installation and application installation, and we should fix it in a separate PR.
cmd/sealer/cmd/cluster/apply.go
Outdated
| return err | ||
| } | ||
| // NOTE: we should continue to apply application after the cluster is applied successfully | ||
| // And it's not needed to prepare the app file repeatedly | ||
| ignorePrepareAppMaterials = true | ||
| ignorePrepareAppMaterials = updated |
There was a problem hiding this comment.
i am ok with you that we need this flag . while, we usually use ignore with a noun or a thing, skip is used with some process ? so SkipPrepareAppMaterials ?
starnop
force-pushed
the
fix-apply
branch
2 times, most recently
from
39f6594 to
8dd54f2
Compare
Signed-off-by: 煜星 <[email protected]>
Describe what this PR does / why we need it
Fix the following two issues:
loadImagewill also launch the appDoes this pull request fix one issue?
Describe how you did it
Describe how to verify it
Special notes for reviews