[docs] Add notes about drift detection #5120

t-kikuc · 2024-08-08T07:19:48Z

What this PR does / why we need it:

~~Fix: DriftDetection is DISABLED by default.~~. Add Note: DriftDetection is enabled by default, but automatic deployment is disabled by default.
Add Note: ignoreFields is supported for only K8sApp.
- Only used in here:
  
  pipecd/pkg/app/piped/driftdetector/kubernetes/detector.go
  
  Line 196 in 7ed87bc
  
  for _, ignoreField := range ddCfg.IgnoreFields {

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Warashi · 2024-08-08T07:23:38Z

docs/content/en/docs-dev/user-guide/configuration-reference.md

@@ -807,7 +807,7 @@ Note: By default, the sum of traffic is rounded to 100. If both `primary` and `c

 | Field | Type | Description | Required |
 |-|-|-|-|
-| ignoreFields | []string | List of fields path in manifests, which its diff should be ignored. | No |
+| ignoreFields | []string | List of fields path in manifests, which its diff should be ignored. This is available for only `KubernetesApp`.This is available for only `KubernetesApp`. | No |


thanks, I fixed

Warashi · 2024-08-08T07:26:18Z

docs/content/en/docs-dev/user-guide/managing-application/configuration-drift-detection.md

@@ -50,7 +50,7 @@ This status means the application is deploying and the configuration drift detec

 ### How to enable

-This feature is automatically enabled for all applications.
+By default, drift detection is **NOT** enabled. You can enable it for each application by configuring application's [OnOutOfSync.disabled](../configuration-reference/#onoutofsync).


The "drift detection" feature is the detection feature, not the reconcile feature.
The option you mention in this PR is the reconcile option; the deployment will be triggered when the sync state is OUT_OF_SYNC.

So, I mean, the drift detection is enabled by default.

Yes, the drift detection is enabled by default. The disabled code is disabled the auto trigger new deployment on OUT_OF_SYNC by default @t-kikuc

thanks both,
yeah, drift detection is enabled by default, and deployment on OutOfSync is not.

I recovered the previous sentence and added a new explanation. 2f0785b

This feature is automatically enabled for all applications. If you want to deploy automatically when `OUT_OF_SYNC` occures, configure each application's [OnOutOfSync](../configuration-reference/#onoutofsync).

@t-kikuc There is a long-time story about this spec 😓
ref: #2752
tl;dr: Previously, we didn't support trigger deployment on OUT_OF_SYNC, which someone thinks that it didn't suit the GitOps spec, so we decided to add that feature. The problem is, if we make that trigger by default, our users at that time will be affected (due to a ton of unexpected deployment triggers whenever OUT_OF_SYNC happens). That's why we make this spec - disable trigger deployment on OUT_OF_SYNC by default. 😄

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Warashi

LGTM

codecov · 2024-08-08T07:53:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.80%. Comparing base (730164c) to head (7b6dba3).
Report is 18 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5120      +/-   ##
==========================================
+ Coverage   22.78%   22.80%   +0.01%     
==========================================
  Files         412      412              
  Lines       43863    43827      -36     
==========================================
  Hits         9996     9996              
+ Misses      33080    33044      -36     
  Partials      787      787

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

khanhtc1202 · 2024-08-08T08:37:10Z

docs/content/en/docs-dev/user-guide/managing-application/configuration-drift-detection.md

@@ -52,6 +52,8 @@ This status means the application is deploying and the configuration drift detec

 This feature is automatically enabled for all applications.

+If you want to deploy automatically when `OUT_OF_SYNC` occures, configure each application's [OnOutOfSync](../configuration-reference/#onoutofsync).


Suggested change

If you want to deploy automatically when `OUT_OF_SYNC` occures, configure each application's [OnOutOfSync](../configuration-reference/#onoutofsync).

If you want to trigger deployment automatically when `OUT_OF_SYNC` occurs, configure each application's [OnOutOfSync](../configuration-reference/#onoutofsync).

typo and re-write 👀

Same for the others

On the second thought, I think this note could lead to confusing situation 🤔 We have place explain how to trigger pipecd deployment here: https://pipecd.dev/docs-v0.48.x/user-guide/managing-application/triggering-a-deployment/#trigger-configuration
Merging this note here could lead to confusion between drift detection feature and trigger feature of pipecd. This page is about enable/disable drift detection feature, nothing related to trigger deployment should be placed here. WDYT? @t-kikuc @Warashi

This document is not a document like a dictionary, so we can write the same thing on several pages.
However, we can avoid confusion by the guide having centralized, with only one page at one thing.
I propose that this notation have a link to the page @khanhtc1202 mentioned, not the configuration reference page.

I propose that this notation have a link to the page @khanhtc1202 mentioned, not the configuration reference page.

I agree

I fixed: a25e438

### How to enable This feature is automatically enabled for all applications. If you want to trigger deployment automatically when `OUT_OF_SYNC` occurs, see [Trigger configuration](./triggering-a-deployment/#trigger-configuration).

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

khanhtc1202 · 2024-08-08T09:44:27Z

docs/content/en/docs-dev/user-guide/managing-application/configuration-drift-detection.md

@@ -52,6 +52,8 @@ This status means the application is deploying and the configuration drift detec

 This feature is automatically enabled for all applications.

+If you want to trigger deployment automatically when `OUT_OF_SYNC` occurs, see [Trigger configuration](./triggering-a-deployment/#trigger-configuration).


This should be a note and place under L57

This feature is automatically enabled for all applications. You can change the checking interval as well as [configure the notification](../../managing-piped/configuring-notifications/) for these events in `piped` configuration. Note: If you want to trigger deployment automatically when `OUT_OF_SYNC` occurs, see [Trigger configuration](./triggering-a-deployment/#trigger-configuration).

It's an easier to read flow

thanks, i fixed 7b6dba3

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

khanhtc1202

Thanks

Warashi

thanks

t-kikuc added 2 commits August 8, 2024 15:59

add note: ignoreFields is available for only K8sApp

8286365

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

add prerequisites: OnOutOfSync.disabled=false

7ed87bc

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc added the area/docs label Aug 8, 2024

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and Warashi as code owners August 8, 2024 07:19

t-kikuc enabled auto-merge (squash) August 8, 2024 07:19

Warashi reviewed Aug 8, 2024

View reviewed changes

t-kikuc disabled auto-merge August 8, 2024 07:34

t-kikuc enabled auto-merge (squash) August 8, 2024 07:34

t-kikuc added 2 commits August 8, 2024 16:40

fix duplicated

e4d9e4c

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

fix default

2f0785b

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

Warashi previously approved these changes Aug 8, 2024

View reviewed changes

t-kikuc changed the title ~~[docs] Fix docs about drift detection~~ [docs] Add notes about drift detection Aug 8, 2024

khanhtc1202 reviewed Aug 8, 2024

View reviewed changes

khanhtc1202 disabled auto-merge August 8, 2024 08:40

link to trigger-configuration

a25e438

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc dismissed Warashi’s stale review via a25e438 August 8, 2024 08:59

khanhtc1202 reviewed Aug 8, 2024

View reviewed changes

fix

7b6dba3

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

t-kikuc enabled auto-merge (squash) August 13, 2024 09:07

t-kikuc closed this Aug 16, 2024

auto-merge was automatically disabled August 16, 2024 04:03
Pull request was closed

t-kikuc reopened this Aug 16, 2024

khanhtc1202 approved these changes Aug 16, 2024

View reviewed changes

t-kikuc enabled auto-merge (squash) August 19, 2024 02:11

Warashi approved these changes Aug 19, 2024

View reviewed changes

t-kikuc merged commit 5c1c633 into master Aug 19, 2024
27 of 28 checks passed

t-kikuc deleted the docs-fix-driftdetection branch August 19, 2024 04:25

github-actions bot mentioned this pull request Aug 26, 2024

Release v0.48.7 #5157

Merged

This was referenced Sep 3, 2024

Update release to v0.48.8 #5176

Merged

Release v0.48.9 #5205

Merged

github-actions bot mentioned this pull request Sep 24, 2024

Update RELEASE to v0.49.0 #5221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[docs] Add notes about drift detection #5120

[docs] Add notes about drift detection #5120

t-kikuc commented Aug 8, 2024 •

edited

Loading

Warashi Aug 8, 2024

t-kikuc Aug 8, 2024

Warashi Aug 8, 2024

khanhtc1202 Aug 8, 2024

t-kikuc Aug 8, 2024

khanhtc1202 Aug 8, 2024

Warashi left a comment

codecov bot commented Aug 8, 2024 •

edited

Loading

khanhtc1202 Aug 8, 2024

khanhtc1202 Aug 8, 2024

khanhtc1202 Aug 8, 2024 •

edited

Loading

Warashi Aug 8, 2024

t-kikuc Aug 8, 2024

t-kikuc Aug 8, 2024

khanhtc1202 Aug 8, 2024

khanhtc1202 Aug 8, 2024

t-kikuc Aug 8, 2024

khanhtc1202 left a comment

Warashi left a comment

	\| ignoreFields \| []string \| List of fields path in manifests, which its diff should be ignored. This is available for only `KubernetesApp`.This is available for only `KubernetesApp`. \| No \|
	\| ignoreFields \| []string \| List of fields path in manifests, which its diff should be ignored. This is available for only `KubernetesApp`. \| No \|

		@@ -52,6 +52,8 @@ This status means the application is deploying and the configuration drift detec

		This feature is automatically enabled for all applications.

		If you want to deploy automatically when `OUT_OF_SYNC` occures, configure each application's [OnOutOfSync](../configuration-reference/#onoutofsync).

	If you want to deploy automatically when `OUT_OF_SYNC` occures, configure each application's [OnOutOfSync](../configuration-reference/#onoutofsync).
	If you want to trigger deployment automatically when `OUT_OF_SYNC` occurs, configure each application's [OnOutOfSync](../configuration-reference/#onoutofsync).

[docs] Add notes about drift detection #5120

[docs] Add notes about drift detection #5120

Conversation

t-kikuc commented Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Warashi left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

Warashi left a comment

Choose a reason for hiding this comment

t-kikuc commented Aug 8, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

khanhtc1202 Aug 8, 2024 •

edited

Loading