Skip to content

Conversation

@jn-av
Copy link
Member

@jn-av jn-av commented Oct 20, 2025

📝 Description

Currently, the merge between parameterSecretRefs and parameters happens at a high level.
Since both objects contain a data field directly underneath, the values of these fields are simply overwritten rather than merged.

This leads to cases where a single field (for example, adminPassword) overwrites the entire data object, which is unintended and causes loss of existing values.


💡 Proposed Change

Implement a recursive (deep) merge for these objects to ensure that:

  • Only explicitly provided fields are updated.
  • Existing data remains intact and is not overwritten unintentionally.

✅ Benefits

  • Prevents accidental data loss when merging.
  • Ensures expected behavior when both parameters and parameterSecretRefs define overlapping keys.
  • Improves reliability and maintainability of parameter handling.

@jn-av jn-av had a problem deploying to pr-e2e-no-approval October 20, 2025 07:56 — with GitHub Actions Failure
@jn-av jn-av changed the title Improve merge logic for parameterSecretRefs and parameters fix: Improve merge logic for parameterSecretRefs and parameters Oct 20, 2025
@jn-av jn-av added the go Pull requests that update go code label Oct 20, 2025
@jn-av jn-av had a problem deploying to pr-e2e-no-approval October 20, 2025 11:06 — with GitHub Actions Failure
@sdischer-sap
Copy link
Member

@jn-av looks already promising ;) Let us know when its ready for review.

@jn-av jn-av temporarily deployed to pr-e2e-no-approval October 21, 2025 09:04 — with GitHub Actions Inactive
@jn-av jn-av temporarily deployed to pr-e2e-no-approval October 21, 2025 09:12 — with GitHub Actions Inactive
@jn-av jn-av marked this pull request as ready for review October 21, 2025 10:53
@jn-av jn-av added the release-notes/bugfix Marks PR as bugfix for release note generation label Oct 21, 2025
Copy link
Contributor

@enrico-kaack-comp enrico-kaack-comp left a comment

Choose a reason for hiding this comment

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

The logic looks great.
This is a single functions without dependencies, would be also a perfect opportunity to test this function itself without the ComplexParameterJson context.

This way it would also be easily visible what this function is expected to do, sort of usage example :)

Copy link
Contributor

@SatabdiG SatabdiG left a comment

Choose a reason for hiding this comment

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

Excellent fix 🚀 with great test coverage. Good work!!

@jn-av jn-av temporarily deployed to pr-e2e-no-approval October 22, 2025 08:27 — with GitHub Actions Inactive
@jn-av jn-av had a problem deploying to pr-e2e-no-approval October 22, 2025 08:56 — with GitHub Actions Failure
@jn-av jn-av temporarily deployed to pr-e2e-no-approval October 22, 2025 11:16 — with GitHub Actions Inactive
Copy link
Member

@sdischer-sap sdischer-sap left a comment

Choose a reason for hiding this comment

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

Can only agree, great work!

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

Labels

go Pull requests that update go code release-notes/bugfix Marks PR as bugfix for release note generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants