Skip to content

Conversation

@nghialv
Copy link
Member

@nghialv nghialv commented Nov 16, 2021

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #2788

Does this PR introduce a user-facing change?:

Allow configuring trigger frequency when application is out_of_sync

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.92%. This pull request decreases coverage by -0.05%.

File Function Base Head Diff
pkg/app/api/grpcapi/piped_api.go PipedAPI.GetDeployment -- 0.00% +0.00%
pkg/app/piped/trigger/determiner.go OnOutOfSyncDeterminer.ShouldTrigger 0.00% 0.00% +0.00%

if time.Since(time.Unix(deployment.CompletedAt, 0)) < appCfg.Trigger.OnOutOfSync.MinWindow.Duration() {
return false, nil
}

Copy link
Member

Choose a reason for hiding this comment

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

tbh, it looks like this determiner will trigger a new deployment whenever

  • no previous triggered deployment (newly added application).
  • time since that latest triggered deployment overs a minWindow of time.

Those two are good to me, but since this determiner is for the OUT_OF_SYNC state, I think we may need logic to trigger a new deployment on app.SyncState.Status is OUT_OF_SYNC. Please correct me if I'm wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks like this determiner will trigger a new deployment whenever

  • no previous triggered deployment (newly added application).
  • time since that latest triggered deployment overs a minWindow of time.

This is correct.

I think we may need logic to trigger a new deployment on app.SyncState.Status is OUT_OF_SYNC.

We don't need that logic because this determiner is accepting only OUT_OF_SYNC candidates.
https://github.com/pipe-cd/pipe/blob/c00e51ac61a93399ccc5e572468c1e23d28d9e8f/pkg/app/piped/trigger/trigger.go#L312
https://github.com/pipe-cd/pipe/blob/master/pkg/app/piped/trigger/trigger.go#L237

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 31.92%. This pull request decreases coverage by -0.05%.

File Function Base Head Diff
pkg/app/api/grpcapi/piped_api.go PipedAPI.GetDeployment -- 0.00% +0.00%
pkg/app/piped/trigger/determiner.go NewOnCommandDeterminer -- 0.00% +0.00%
pkg/app/piped/trigger/determiner.go NewOnOutOfSyncDeterminer -- 0.00% +0.00%
pkg/app/piped/trigger/determiner.go OnOutOfSyncDeterminer.ShouldTrigger 0.00% 0.00% +0.00%

@nakabonne
Copy link
Member

Looks neat
/lgtm

@nghialv
Copy link
Member Author

nghialv commented Nov 17, 2021

@khanhtc1202 Can you take a look at this?

@khanhtc1202
Copy link
Member

Sorry for my miss, way to go 🚀
/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a field to control the trigger frequency.

5 participants