kpt pkg update overwrites local changes to Kptfile

[jafcrocker]

Expected behavior

Performing a kpt pkg update should merge changes to the Kptfile committed to the local directory with changes from the source package.

Actual behavior

Performing a kpt pkg update overwrites changes to the Kptfile committed to the local directory with changes from the source directory.

Information

Items of possible interest:

• Kpt version: 1.0.0-beta.5
• The changes are made to the Kptfile.
• The ref to the remote package is an annotated git tag, not a branch.
• The changes consist of adding the same root field of the Kptfile in both cases (i.e., .pipeline)

Steps to reproduce the behavior

1) Inspect the package source. Note the changes to the Kptfile

% cd ~/src/REDACTED-SRC-REPO
REDACTED-SRC-REPO % git show pkg/gcp-data-project/v0.2.0
tag pkg/gcp-data-project/v0.2.0
Tagger: REDACTED
Date:   Mon Aug 23 17:42:49 2021 +0000

Automated via Cloud Build pipeline

commit 8d3eda19518d44ffa60a66ab6850b412ac4181bf (tag: pkg/gcp-data-project/v0.2.0)
Merge: 5bd274f 42705cc
Author: REDACTED
Date:   Mon Aug 23 12:42:16 2021 -0500

    Merge pull request #45 from wpengine/REDACTED/CTLST-1241

REDACTED-SRC-REPO % git show pkg/gcp-data-project/v0.2.1
tag pkg/gcp-data-project/v0.2.1
Tagger: REDACTED
Date:   Mon Aug 30 21:12:15 2021 +0000

Automated via Cloud Build pipeline

commit 6679457c518e11f0114554ba159a62e801fe4bc8 (HEAD, tag: pkg/gcp-data-project/v0.2.1)
Merge: 6b21e71 372b8e8
Author: REDACTED
Date:   Mon Aug 30 16:11:40 2021 -0500

    Merge pull request #49 from wpengine/CTLST-1254.2
    
    CTLST-1254 - update gcp-data-project to use gcp-project v0.2.1

REDACTED-SRC-REPO % git diff pkg/gcp-data-project/v0.2.0 pkg/gcp-data-project/v0.2.1 -- pkg/gcp-data-project/Kptfile

diff --git a/pkg/gcp-data-project/Kptfile b/pkg/gcp-data-project/Kptfile
index d3351d3..8a51bf6 100644
--- a/pkg/gcp-data-project/Kptfile
+++ b/pkg/gcp-data-project/Kptfile
@@ -4,3 +4,8 @@ metadata:
   name: gcp-data-project
 info:
   description: sample description
+pipeline:
+  mutators:
+    - image: gcr.io/kpt-fn/set-annotations:unstable
+      configMap:
+        wpengine.com/REDACTED-SRC-REPO.pkg-version.gcp-data-project: 0.2.1

2) Change to the repo that instantiates the package; switch to the commit that updates the version.

REDACTED-SRC-REPO % cd ~/src/cp 
cp % git co 2b021a4947028b9b6824230ecf0769cd3613f724

HEAD is now at 2b021a4 Update Kptfile to gcp-data-project v0.2.1
cp % cd infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev

3) Inspect the current state of the Kptfile

wpe-ctlst-hello-world-dev % git show HEAD

commit 2b021a4947028b9b6824230ecf0769cd3613f724 (HEAD)
Author: REDACTED
Date:   Mon Aug 30 16:46:31 2021 -0500

    Update Kptfile to gcp-data-project v0.2.1

diff --git a/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile b/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile
index d64aa84..840beb1 100644
--- a/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile
+++ b/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile
@@ -7,7 +7,7 @@ upstream:
   git:
     repo: ssh://git@github.com/wpengine/REDACTED-SRC-REPO
     directory: /pkg/gcp-data-project
-    ref: v0.2.0
+    ref: v0.2.1
   updateStrategy: resource-merge
 upstreamLock:
   type: git
wpe-ctlst-hello-world-dev % cat Kptfile

apiVersion: kpt.dev/v1
kind: Kptfile
metadata:
  name: wpe-ctlst-hello-world-dev
upstream:
  type: git
  git:
    repo: ssh://git@github.com/wpengine/REDACTED-SRC-REPO
    directory: /pkg/gcp-data-project
    ref: v0.2.1
  updateStrategy: resource-merge
upstreamLock:
  type: git
  git:
    repo: ssh://git@github.com/wpengine/REDACTED-SRC-REPO
    directory: /pkg/gcp-data-project
    ref: pkg/gcp-data-project/v0.2.0
    commit: 0580598ccf4e914bd3e076931a43f64a94bd270f
info:
  description: sample description
pipeline:
  mutators:
    - image: gcr.io/kpt-fn/apply-setters:v0.1
      configPath: fn-cfg-setters.yaml

4) Update the package

wpe-ctlst-hello-world-dev % kpt pkg update .
Package "wpe-ctlst-hello-world-dev":
Fetching upstream from ssh://git@github.com/wpengine/REDACTED-SRC-REPO@v0.2.1
From ssh://github.com/wpengine/REDACTED-SRC-REPO
 * tag               pkg/gcp-data-project/v0.2.1 -> FETCH_HEAD
Fetching origin from ssh://git@github.com/wpengine/REDACTED-SRC-REPO@v0.2.1
From ssh://github.com/wpengine/REDACTED-SRC-REPO
 * branch            0580598ccf4e914bd3e076931a43f64a94bd270f -> FETCH_HEAD
Updating package "wpe-ctlst-hello-world-dev" with strategy "resource-merge".
Updating package "gcp-project" with strategy "resource-merge".

Package "wpe-ctlst-hello-world-dev/gcp-project":
Fetching upstream from ssh://git@github.com/wpengine/REDACTED-SRC-REPO@v0.2.1
From ssh://github.com/wpengine/REDACTED-SRC-REPO
 * tag               pkg/gcp-project/v0.2.1 -> FETCH_HEAD
Fetching origin from ssh://git@github.com/wpengine/REDACTED-SRC-REPO@v0.2.1
From ssh://github.com/wpengine/REDACTED-SRC-REPO
 * branch            80b0649ed24503daec0a570c466305fdab09fa50 -> FETCH_HEAD
Updating package "gcp-project" with strategy "resource-merge".

Updated 2 package(s).

5) Inspect the results of the update.

Note that the local changes (i.e., apply-setters) were overwritten by the changes from the source package (i.e., set-annotations).

wpe-ctlst-hello-world-dev % git diff Kptfile
diff --git a/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile b/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile
index 840beb1..7aa5760 100644
--- a/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile
+++ b/infra/krm/catalyst-platform/data-projects/environments/dev/wpe-ctlst-hello-world-dev/Kptfile
@@ -14,11 +14,12 @@ upstreamLock:
   git:
     repo: ssh://git@github.com/wpengine/REDACTED-SRC-REPO
     directory: /pkg/gcp-data-project
-    ref: pkg/gcp-data-project/v0.2.0
-    commit: 0580598ccf4e914bd3e076931a43f64a94bd270f
+    ref: pkg/gcp-data-project/v0.2.1
+    commit: edc875a2f90ad28020457f7de3c78f60d562e1a2
 info:
   description: sample description
 pipeline:
   mutators:
-    - image: gcr.io/kpt-fn/apply-setters:v0.1
-      configPath: fn-cfg-setters.yaml
+    - image: gcr.io/kpt-fn/set-annotations:unstable
+      configMap:
+        wpengine.com/REDACTED-SRC-REPO.pkg-version.gcp-data-project: 0.2.1

[phanimarupaka]

@jafcrocker This is a known issue with merging pipeline section in Kptfile. Please refer to this comment for the reason behind this behavior. While this item is on our backlog, we want to know how blocked are you on this issue to prioritize it appropriately.

However, I want to know use-case further and see if I can help with a possible workaround. To summarize the issue:

1. The Kptfile in pkg/gcp-data-project/v0.2.0 doesn't have set-annotation function.
2. You forked the pkg/gcp-data-project/v0.2.0 on to local.
3. You added apply-setters function to local Kptfile and committed.
4. Package maintainer added set-annotation function and bumped up version to pkg/gcp-data-project/v0.2.1
5. Updating the package dropped your locally added apply-setters function.

I think you might have added the apply-setters function back manually after kpt pkg update. Is this a repetitive thing ? Are you running into this problem time and again ?

cc @mikebz

[jafcrocker]

@phanimarupaka

1. Thanks for pointing out the underlying problem; I appreciate the background.
2. Your summary is perfect.
3. You speculated that I might have manually added apply-setters after kpt pkg update; I did not.
4. We are not repeatedly running into this problem; this is a one time issue.

Regarding (4), the reasons are twofold:

1. We've not updated the base package.
2. We've since moved to a model in which we do not manually edit any of the forked resources. Instead we use a Kptfile in a parent directory with a pipeline which applies the edits we need.

[droot]

@jafcrocker I have a few questions (if it is okay for you to share them here):

We've since moved to a model in which we do not manually edit any of the forked resources.

What factors influenced your decision to moved to the new model?

Instead we use a Kptfile in a parent directory with a pipeline which applies the edits we need.

Programmatic customizations can be done by functions in the parent, but I am curious about the manual edits, how are you achieving those.

[jafcrocker]

@droot

What factors influenced your decision to moved to the new model?

1. We feel that this model reduces cognitive load. That is, in effect it abstracts away the contents of the forked package. To find the deltas from the forked package, you only look in the parent package, rather than digging through git history. There's a cost, of course, in expressing the desired output in terms of a base package and edits, but it feels like it better expresses the intent than editing the forked resources.

2. We have had some surprises (in addition to this issue) in the kpt conflict resolution. It's quite possible they have all had the same root cause; we haven't kept track. In aggregate those problems were enough to make us not want to rely on it. Making edits in the parent directory means that we don't need to rely on kpt conflict resolution.

Programmatic customizations can be done by functions in the parent, but I am curious about the manual edits, how are you achieving those?

We have been able to make all necessary edits via a combination of existing and custom kpt functions in the parent. We commonly use search-replace, create-setters/apply-setters, and set-annotations/set-namespace/set-labels. We have a custom function that allows us to apply kustomize style patches. And when all else fails, we can always use starlark.

We haven't used this model extensively, so we may yet run into problems, but so far it's working out well.

[phanimarupaka]

@jafcrocker This request has been addressed. Please try out beta.12 version of kpt which will be released soon.