Allow triggering deployment when application is at out-of-sync state #2777

nghialv · 2021-11-11T16:04:51Z

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Allow triggering deployment when application is at out-of-sync state

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/piped/trigger/trigger.go

pkg/app/piped/trigger/determiner.go

pkg/app/piped/trigger/trigger.go

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/piped/trigger/determiner.go

nakabonne · 2021-11-12T02:27:25Z

Test TestLambdaDeploymentConfig seems to be failed

ono-max · 2021-11-12T05:07:00Z

When I tried to this change in my local environment (missing .pipe.yaml), I got a lot of notification in Slack at once as follows:

Should we sleep between notification?

nghialv · 2021-11-15T00:40:48Z

When I tried to this change in my local environment (missing .pipe.yaml), I got a lot of notification in Slack at once as follows:

Nice catch!

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/piped/trigger/trigger.go

pkg/app/piped/trigger/determiner.go

pipecd-bot · 2021-11-15T01:24:16Z

Code coverage for golang is 32.04%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/app/piped/planpreview/builder.go	loadDeploymentConfiguration	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	reportMostRecentlyTriggeredDeployment	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	determiners.Determiner	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnCommandDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnOutOfSyncDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	NewOnCommitDeterminer	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnCommitDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkRepoCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listCommandCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listOutOfSyncCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listCommitCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.notifyDeploymentTriggered	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.notifyDeploymentTriggerFailed	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	loadDeploymentConfiguration	`--`	`0.00%`	`+0.00%`
pkg/app/piped/planpreview/builder.go	builder.findTriggerApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.triggerDeployment	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.Run	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.updateRepoToLatest	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.reportMostRecentlyTriggeredDeployment	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.reportDeploymentFailed	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.getNotification	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	NewDeterminer	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.shouldTriggerOnCommit	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	loadDeploymentConfiguration	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkNewCommands	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkNewCommits	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.syncApplication	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listApplications	`0.00%`	`--`	`+-0.00%`

pipecd-bot · 2021-11-15T01:35:16Z

Code coverage for golang is 32.04%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/app/piped/planpreview/builder.go	loadDeploymentConfiguration	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	reportMostRecentlyTriggeredDeployment	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	determiners.Determiner	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnCommandDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnOutOfSyncDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	NewOnCommitDeterminer	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnCommitDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkRepoCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listCommandCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listOutOfSyncCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listCommitCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.notifyDeploymentTriggered	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.notifyDeploymentTriggerFailed	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	loadDeploymentConfiguration	`--`	`0.00%`	`+0.00%`
pkg/app/piped/planpreview/builder.go	builder.findTriggerApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.triggerDeployment	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.Run	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.updateRepoToLatest	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.reportMostRecentlyTriggeredDeployment	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.reportDeploymentFailed	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.getNotification	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	NewDeterminer	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.shouldTriggerOnCommit	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	loadDeploymentConfiguration	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkNewCommands	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkNewCommits	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.syncApplication	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listApplications	`0.00%`	`--`	`+-0.00%`

nakabonne · 2021-11-15T03:06:11Z

/lgtm

khanhtc1202 · 2021-11-15T04:40:25Z

pkg/config/deployment.go

+	// Whether to exclude application from triggering target
+	// when application is at OUT_OF_SYNC state.
+	// Default is true.
+	Disabled bool `json:"disabled,omitempty" default:"true"`


nits, with this default:"true" labels on boolean type value, even if users explicitly set spec.input.trigger.onOutOfSync.Disable: false, they still can not enable trigger new deployment on out of sync.

Sorry, what do you mean?
Why " they still can not enable trigger new deployment on out of sync"?

I think I got your point. The "defaults" lib is configuring that field to be true whenever its value is false, doesn't matter if it was specified explicitly or not. So its final value always will be true, right?

Yes, so I think we may need *bool for this case 🤔

Right. *bool or defined a new Bool type for that case. Let me fix it. Nice catch!

btw, other places (such as autoRollback configuration) have this issue as well, so addressing that issue by a separated PR is fine, but I just want to explicitly mention it to ensure everyone aware of that issue.

Right. *bool or defined a new Bool type for that case. Let me fix it. Nice catch!

Thanks, will fix other places by separated PR later 👍

@khanhtc1202 I have added a TODO to this PR. And let me fix all of those things by a separate PR.

🙆‍♀️

pkg/app/piped/trigger/determiner.go

pipecd-bot · 2021-11-15T06:09:38Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Remove deprecated `appCfg.TriggerPaths` configuration.

https://github.com/pipe-cd/pipe/blob/12396182979ad9287880d483f882b9c81ad519a5/pkg/app/piped/trigger/determiner.go#L136-L139

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.

2. Find a better way to skip the CANCELLED error log while shutting down.

https://github.com/pipe-cd/pipe/blob/12396182979ad9287880d483f882b9c81ad519a5/pkg/app/piped/trigger/trigger.go#L209-L212

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.

3. Ensure that the given value will not be overridden by the default one.

https://github.com/pipe-cd/pipe/blob/12396182979ad9287880d483f882b9c81ad519a5/pkg/config/deployment.go#L94-L97

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.

4. Add a field to control the trigger frequency.

https://github.com/pipe-cd/pipe/blob/12396182979ad9287880d483f882b9c81ad519a5/pkg/config/deployment.go#L96-L99

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.

pipecd-bot · 2021-11-15T06:16:31Z

Code coverage for golang is 32.04%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/app/piped/planpreview/builder.go	loadDeploymentConfiguration	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	reportMostRecentlyTriggeredDeployment	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	determiners.Determiner	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnCommandDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnOutOfSyncDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	NewOnCommitDeterminer	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/determiner.go	OnCommitDeterminer.ShouldTrigger	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkRepoCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listCommandCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listOutOfSyncCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listCommitCandidates	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.notifyDeploymentTriggered	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.notifyDeploymentTriggerFailed	`--`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	loadDeploymentConfiguration	`--`	`0.00%`	`+0.00%`
pkg/app/piped/planpreview/builder.go	builder.findTriggerApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.triggerDeployment	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.Run	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.updateRepoToLatest	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.reportMostRecentlyTriggeredDeployment	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.reportDeploymentFailed	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/deployment.go	Trigger.getNotification	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	NewDeterminer	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.ShouldTrigger	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	Determiner.shouldTriggerOnCommit	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/determiner.go	loadDeploymentConfiguration	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkNewCommands	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.checkNewCommits	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.syncApplication	`0.00%`	`--`	`+-0.00%`
pkg/app/piped/trigger/trigger.go	Trigger.listApplications	`0.00%`	`--`	`+-0.00%`

khanhtc1202 · 2021-11-15T06:20:12Z

Nice work 🚀
/approve

pipecd-bot · 2021-11-15T06:20:15Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

pipecd-bot added the area/piped label Nov 11, 2021

pipecd-bot requested review from nakabonne and ono-max November 11, 2021 16:04

pipecd-bot added the size/XL label Nov 11, 2021

pipecd-bot reviewed Nov 11, 2021

View reviewed changes

pkg/app/piped/trigger/determiner.go Outdated Show resolved Hide resolved

pkg/app/piped/trigger/determiner.go Outdated Show resolved Hide resolved

nghialv added 3 commits November 15, 2021 09:54

Allow triggering deployment when application is at out-of-sync state

74a5b8f

Fix tests

f558738

Fix tests

bb43c35

nghialv force-pushed the trigger-next branch from 217e8d0 to bb43c35 Compare November 15, 2021 01:17

pipecd-bot reviewed Nov 15, 2021

View reviewed changes

pkg/app/piped/trigger/trigger.go Show resolved Hide resolved

pkg/app/piped/trigger/determiner.go Outdated Show resolved Hide resolved

pkg/app/piped/trigger/determiner.go Outdated Show resolved Hide resolved

Remove unused parameters

13450f6

Refine notification

0ed5f99

pipecd-bot added the lgtm label Nov 15, 2021

khanhtc1202 reviewed Nov 15, 2021

View reviewed changes

pkg/app/piped/trigger/determiner.go Show resolved Hide resolved

Update deployment.go

1239618

pipecd-bot removed the lgtm label Nov 15, 2021

pipecd-bot added the approved label Nov 15, 2021

pipecd-bot merged commit a50cc23 into master Nov 15, 2021

pipecd-bot deleted the trigger-next branch November 15, 2021 06:20

This was referenced Dec 1, 2021

Update doc for deployment triggering #2867

Merged

Temporarily disable appconfig reporter in Piped #2868

Merged

Allow triggering deployment when application is at out-of-sync state #2777

Allow triggering deployment when application is at out-of-sync state #2777

Uh oh!

Conversation

nghialv commented Nov 11, 2021

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nakabonne commented Nov 12, 2021

Uh oh!

ono-max commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nghialv commented Nov 15, 2021

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pipecd-bot commented Nov 15, 2021

Uh oh!

pipecd-bot commented Nov 15, 2021

Uh oh!

nakabonne commented Nov 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pipecd-bot commented Nov 15, 2021

1. Remove deprecated appCfg.TriggerPaths configuration.

This was created by todo plugin since "TODO:" was found in 1239618 when #2777 was merged. cc: @nghialv.

2. Find a better way to skip the CANCELLED error log while shutting down.

This was created by todo plugin since "TODO:" was found in 1239618 when #2777 was merged. cc: @nghialv.

3. Ensure that the given value will not be overridden by the default one.

This was created by todo plugin since "TODO:" was found in 1239618 when #2777 was merged. cc: @nghialv.

4. Add a field to control the trigger frequency.

This was created by todo plugin since "TODO:" was found in 1239618 when #2777 was merged. cc: @nghialv.

Uh oh!

pipecd-bot commented Nov 15, 2021

Uh oh!

khanhtc1202 commented Nov 15, 2021

Uh oh!

pipecd-bot commented Nov 15, 2021

Uh oh!

Reviewers

Assignees

Labels

ono-max commented Nov 12, 2021 •

edited

Loading

1. Remove deprecated `appCfg.TriggerPaths` configuration.

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.

This was created by todo plugin since "TODO:" was found in `1239618` when #2777 was merged. cc: @nghialv.