Rewrite rollout controller code #102

zmberg · 2022-11-11T10:47:03Z

Ⅰ. Describe what this PR does

Rewrite rollout controller code

pkg/controller/rollout/rollout_canary.go

sonatype-lift · 2022-11-15T12:47:28Z

pkg/controller/rollout/rollout_status.go

+				ObservedWorkloadGeneration: workload.Generation,
+				PodTemplateHash:            workload.PodTemplateHash,
+				CanaryRevision:             workload.CanaryRevision,
+				StableRevision: 			workload.StableRevision,


gofmt: File is not gofmt-ed with -s

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-11-17T08:25:03Z

pkg/controller/rollout/rollout_status.go

+	"context"
+	"fmt"
+	"reflect"
+	"sigs.k8s.io/controller-runtime/pkg/client"


goimports: File is not goimports-ed

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-11-22T11:52:51Z

pkg/controller/rollout/rollout_controller_test.go

 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	utilpointer "k8s.io/utils/pointer"
 )

 var (
-	scheme *runtime.Scheme
+	scheme                              *runtime.Scheme
+	nginxIngressAnnotationDefaultPrefix = "nginx.ingress.kubernetes.io"


U1000: var nginxIngressAnnotationDefaultPrefix is unused

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-11-22T11:52:56Z

pkg/controller/rollout/rollout_controller_test.go

@@ -17,59 +17,130 @@ limitations under the License.
 package rollout

 import (
+	"fmt"


goimports: File is not goimports-ed

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift · 2022-11-22T11:53:03Z

pkg/trafficrouting/manager_test.go

+import (
+	"context"
+	"fmt"
+	"github.com/openkruise/rollouts/pkg/util/configuration"


goimports: File is not goimports-ed

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

codecov-commenter · 2022-11-23T04:11:12Z

Codecov Report

Merging #102 (cd25c84) into master (c0b1fea) will increase coverage by 9.05%.
The diff coverage is 41.88%.

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   36.36%   45.41%   +9.05%     
==========================================
  Files          40       37       -3     
  Lines        3556     3543      -13     
==========================================
+ Hits         1293     1609     +316     
+ Misses       2059     1652     -407     
- Partials      204      282      +78

Flag	Coverage Δ
unittests	`45.41% <41.88%> (+9.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/controller/rollout/rollout_controller.go	`10.29% <0.00%> (-2.21%)`	⬇️
pkg/controller/rollout/rollout_event_handler.go	`48.57% <0.00%> (ø)`
pkg/trafficrouting/network/gateway/gateway.go	`53.27% <0.00%> (ø)`
pkg/util/condition.go	`0.00% <0.00%> (ø)`
pkg/util/controller_finder.go	`0.00% <0.00%> (ø)`
pkg/util/lua_configuration.go	`37.50% <ø> (ø)`
pkg/util/rollout_utils.go	`3.50% <0.00%> (+0.33%)`	⬆️
pkg/controller/rollout/rollout_status.go	`20.00% <20.00%> (ø)`
pkg/trafficrouting/network/ingress/ingress.go	`61.80% <25.00%> (ø)`
pkg/controller/rollout/rollout_canary.go	`43.46% <43.46%> (ø)`
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sonatype-lift · 2022-11-23T10:48:52Z

pkg/controller/rollout/rollout_controller_test.go

 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	utilpointer "k8s.io/utils/pointer"
 )

 var (
-	scheme *runtime.Scheme
+	scheme                              *runtime.Scheme


gofmt: File is not gofmt-ed with -s

ℹ️ Learn about @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

veophi · 2022-12-06T12:22:18Z

pkg/controller/rollout/rollout_canary.go

+	steps := len(c.Rollout.Spec.Strategy.Canary.Steps)
+	cond := util.GetRolloutCondition(*c.NewStatus, v1alpha1.RolloutConditionProgressing)
+	// need manual confirmation
+	if currentStep.Pause.Duration == nil {


If currentSetp == "100%", should we still need this manual confirmation?

The situation is a bit complicated, because there are two fields, weight and replicas, if one of them is 100% and the other is not, it is difficult to judge.

pkg/controller/rollout/rollout_canary.go

pkg/controller/rollout/rollout_controller.go

pkg/controller/rollout/rollout_progressing.go

pkg/controller/rollout/rollout_canary.go

pkg/controller/rollout/rollout_status.go

pkg/controller/rollout/rollout_controller.go

pkg/trafficrouting/network/ingress/ingress.go

pkg/controller/rollout/rollout_status.go

furykerry · 2022-12-15T01:40:36Z

/lgtm

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>

furykerry · 2022-12-15T05:35:29Z

/lgtm

furykerry · 2022-12-15T05:57:50Z

/lgtm

furykerry · 2022-12-16T14:55:52Z

/approve

kruise-bot · 2022-12-16T14:55:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: furykerry

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:

~~OWNERS~~ [furykerry]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zmberg requested review from furykerry and veophi November 11, 2022 10:47

kruise-bot added the size/XXL label Nov 11, 2022

sonatype-lift bot reviewed Nov 11, 2022

View reviewed changes

pkg/controller/rollout/rollout_canary.go Outdated Show resolved Hide resolved

zmberg force-pushed the rewrite-for-rollout-controller branch 2 times, most recently from 5aab2b4 to 0641a29 Compare November 15, 2022 12:38

sonatype-lift bot reviewed Nov 15, 2022

View reviewed changes

kruise-bot added the needs-rebase label Nov 16, 2022

zmberg force-pushed the rewrite-for-rollout-controller branch from 0641a29 to 9ab695d Compare November 17, 2022 07:13

sonatype-lift bot reviewed Nov 17, 2022

View reviewed changes

zmberg force-pushed the rewrite-for-rollout-controller branch 2 times, most recently from 7d7ad16 to 859c5f5 Compare November 22, 2022 11:45

kruise-bot removed the needs-rebase label Nov 22, 2022

sonatype-lift bot reviewed Nov 22, 2022

View reviewed changes

zmberg force-pushed the rewrite-for-rollout-controller branch 3 times, most recently from 9be3169 to bb454df Compare November 23, 2022 10:48

sonatype-lift bot reviewed Nov 23, 2022

View reviewed changes

zmberg force-pushed the rewrite-for-rollout-controller branch 3 times, most recently from 7cfa022 to 7889c9f Compare November 24, 2022 02:57

kruise-bot added the needs-rebase label Dec 1, 2022

zmberg force-pushed the rewrite-for-rollout-controller branch from 7889c9f to 8900fe1 Compare December 6, 2022 06:08

kruise-bot removed the needs-rebase label Dec 6, 2022

veophi reviewed Dec 6, 2022

View reviewed changes

furykerry reviewed Dec 7, 2022

View reviewed changes

pkg/controller/rollout/rollout_controller.go Outdated Show resolved Hide resolved

pkg/controller/rollout/rollout_progressing.go Outdated Show resolved Hide resolved

pkg/controller/rollout/rollout_progressing.go Outdated Show resolved Hide resolved

zmberg force-pushed the rewrite-for-rollout-controller branch from 8900fe1 to 7868fd6 Compare December 7, 2022 05:39

zmberg force-pushed the rewrite-for-rollout-controller branch from 7868fd6 to 1b655b9 Compare December 9, 2022 09:36

furykerry reviewed Dec 9, 2022

View reviewed changes

pkg/controller/rollout/rollout_progressing.go Outdated Show resolved Hide resolved

furykerry reviewed Dec 9, 2022

View reviewed changes

zmberg force-pushed the rewrite-for-rollout-controller branch 4 times, most recently from 25d861a to a5fba0b Compare December 14, 2022 12:38

furykerry reviewed Dec 14, 2022

View reviewed changes

pkg/trafficrouting/network/ingress/ingress.go Outdated Show resolved Hide resolved

pkg/controller/rollout/rollout_status.go Outdated Show resolved Hide resolved

pkg/controller/rollout/rollout_status.go Outdated Show resolved Hide resolved

zmberg force-pushed the rewrite-for-rollout-controller branch from a5fba0b to db67699 Compare December 14, 2022 13:05

kruise-bot assigned furykerry Dec 15, 2022

kruise-bot added the lgtm label Dec 15, 2022

Rewrite rollout controller code

fbf442e

Signed-off-by: liheng.zms <liheng.zms@alibaba-inc.com>

zmberg force-pushed the rewrite-for-rollout-controller branch from db67699 to fbf442e Compare December 15, 2022 02:53

kruise-bot removed the lgtm label Dec 15, 2022

kruise-bot added the lgtm label Dec 15, 2022

kruise-bot added the approved label Dec 16, 2022

kruise-bot merged commit 973e39b into openkruise:master Dec 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite rollout controller code #102

Rewrite rollout controller code #102

zmberg commented Nov 11, 2022

sonatype-lift bot Nov 15, 2022

sonatype-lift bot Nov 17, 2022

sonatype-lift bot Nov 22, 2022

sonatype-lift bot Nov 22, 2022

sonatype-lift bot Nov 22, 2022

codecov-commenter commented Nov 23, 2022 •

edited

Loading

sonatype-lift bot Nov 23, 2022

veophi Dec 6, 2022

zmberg Dec 7, 2022

furykerry commented Dec 15, 2022

furykerry commented Dec 15, 2022

furykerry commented Dec 15, 2022

furykerry commented Dec 16, 2022

kruise-bot commented Dec 16, 2022

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Rewrite rollout controller code #102

Rewrite rollout controller code #102

Conversation

zmberg commented Nov 11, 2022

Ⅰ. Describe what this PR does

sonatype-lift bot Nov 15, 2022

Choose a reason for hiding this comment

sonatype-lift bot Nov 17, 2022

Choose a reason for hiding this comment

sonatype-lift bot Nov 22, 2022

Choose a reason for hiding this comment

sonatype-lift bot Nov 22, 2022

Choose a reason for hiding this comment

sonatype-lift bot Nov 22, 2022

Choose a reason for hiding this comment

codecov-commenter commented Nov 23, 2022 • edited Loading

Codecov Report

sonatype-lift bot Nov 23, 2022

Choose a reason for hiding this comment

veophi Dec 6, 2022

Choose a reason for hiding this comment

zmberg Dec 7, 2022

Choose a reason for hiding this comment

furykerry commented Dec 15, 2022

furykerry commented Dec 15, 2022

furykerry commented Dec 15, 2022

furykerry commented Dec 16, 2022

kruise-bot commented Dec 16, 2022

codecov-commenter commented Nov 23, 2022 •

edited

Loading