Skip to content

Support patching for kustomize#1835

Merged
octonautcal merged 65 commits intomainfrom
cal/md-1462-kustomize-files-do-not-support-patches-only-newtag-could-we
Apr 2, 2026
Merged

Support patching for kustomize#1835
octonautcal merged 65 commits intomainfrom
cal/md-1462-kustomize-files-do-not-support-patches-only-newtag-could-we

Conversation

@octonautcal
Copy link
Copy Markdown
Contributor

@octonautcal octonautcal commented Mar 17, 2026

Adds support for Kustomize patching in ArgoCD deployments. The PR implements patch discovery and image replacement capabilities for different patch types:

  • JSON6902 patch
  • Strategic merge patch
  • Inline JSON patch

Testing:

Tested local Calamari with an local Octopus Server instance:

Screenshot 2026-03-24 at 2 01 42 pm

The changed commit a2defd6f2a showed that it consistently updated the image tags properly across all scenarios.

@octonautcal octonautcal force-pushed the cal/md-1462-kustomize-files-do-not-support-patches-only-newtag-could-we branch from 0a3e4da to 2044772 Compare March 17, 2026 05:01
@octonautcal octonautcal force-pushed the cal/md-1462-kustomize-files-do-not-support-patches-only-newtag-could-we branch from 2044772 to dbbb1a9 Compare March 17, 2026 05:10
@octonautcal octonautcal force-pushed the cal/md-1462-kustomize-files-do-not-support-patches-only-newtag-could-we branch from dc561ba to 51181a3 Compare March 23, 2026 04:28
@octonautcal octonautcal marked this pull request as ready for review March 24, 2026 03:03
@rain-on
Copy link
Copy Markdown
Contributor

rain-on commented Mar 24, 2026

There's a few smells in the KustomizeUpdater which should be easy to fix. But a few general thoughts:

  1. The selection of files process is really nice 👍
  2. Pulling Update into the KustomizeUpdater is not great - I know its needed for this design to work, as you need currentFile - but it'd be good to avoid.
  3. The currentFile property feels a bit dangerous - esp given that it needs to set and unset - means we've got something wrong with lifetime management I suspect.
    3.1 On that topic - we shouldn't need currentFile - yes it can be used as a short-circuit to determine the type of file being updated - but we could infer the file-type from the provided content (and hide it behind a function so we don't need to look at it too often).
  4. KustomizeUpdater might need to grow a bit, but going from ~64 --> ~450 is probably too much, hopefully a lot of that could be extracted to a separate class.

}


internal PatchType? DeterminePatchTypeFromFile(string content)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

generally we don't use internal - I'd look at removing them across this whole PR I think

Copy link
Copy Markdown
Contributor

@rain-on rain-on left a comment

Choose a reason for hiding this comment

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

approved with comments.

/// Handles updating container image references in inline patches within kustomization.yaml files.
/// The patches field contains an array of patch objects with inline patch operations.
/// </summary>
public class InlineJsonPatchImageReplacer : IContainerImageReplacer
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

dont love the "InlineJsonPatch" - Maybe just InlinepatchImageReplacer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

/// <summary>
/// Helper class for loading and parsing YAML content
/// </summary>
public static class YamlStreamLoader
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

kinda surprised we need this class - but also, not really :)

{
public static class YamlStreamLoader
{
public static YamlStream? TryLoad(string yamlContent, ILog log, string context)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we need this, if we have the YamlStreamLoader?

// We know this won't be null after parse
new (ContainerImageReference.FromReferenceString("nginx:1.25", ArgoCDConstants.DefaultContainerRegistry)),
new (ContainerImageReference.FromReferenceString("busybox:stable", "my-registry.com")),
new (ContainerImageReference.FromReferenceString("my-registry.com/busybox:stable", ArgoCDConstants.DefaultContainerRegistry)),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this shouldn't have needed changing - this example should still work

@octonautcal octonautcal enabled auto-merge (squash) April 2, 2026 06:20
@octonautcal octonautcal merged commit 1ad8f21 into main Apr 2, 2026
33 checks passed
@octonautcal octonautcal deleted the cal/md-1462-kustomize-files-do-not-support-patches-only-newtag-could-we branch April 2, 2026 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants