Skip to content

[K8s Plugin] Enhance Manifest struct with deep copy functionality and conversion methods #5803

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 12, 2025

Conversation

Warashi
Copy link
Member

@Warashi Warashi commented May 8, 2025

What this PR does:

  • refactor (Manifest).ConvertToStructuredObject to use runtime.DefaultUnstructuredConverter.FromUnstructured.
  • add deep-copy functionality of manifests
    • and use it to resolve TODOs
  • add FromStructuredObject to create Manifest from k8s structured object

Why we need it:

  • refactoring of ConvertToStructuredObject
    • it's better to use the provided way
  • deep-copy
    • to avoid unexpected side-effective modifications
  • FromStructuredObject
    • to use in the following scenario
      1. convert Manifest to structured object with ConvertToStructuredObject
      2. modify it
      3. re-convert modified object with FromStructuredObject

Which issue(s) this PR fixes:

Part of #5764

Does this PR introduce a user-facing change?:

  • How are users affected by this change:
  • Is this breaking change:
  • How to migrate (if breaking change):

Warashi added 3 commits May 8, 2025 13:43
ConvertToStructuredObject changed the behavior around secret data.
Now it decodes the base64 encoded data into the provided object.

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Copy link

codecov bot commented May 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.24%. Comparing base (33670a8) to head (4524f18).
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5803      +/-   ##
==========================================
+ Coverage   27.14%   27.24%   +0.10%     
==========================================
  Files         508      508              
  Lines       53803    53823      +20     
==========================================
+ Hits        14603    14665      +62     
+ Misses      38084    38036      -48     
- Partials     1116     1122       +6     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

expected: "74bd68bm66",
expected: "2k5dh9h9h9",
Copy link
Member Author

Choose a reason for hiding this comment

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

ConvertToStructuredObject changed the behavior around secret data.
Now, it decodes the base64 encoded data into the provided object.
This behavior change makes the calculated hash to be another one.

Copy link
Member

Choose a reason for hiding this comment

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

[ask] Just for clarify, does it affect the hash annotation for each secret when migrating to piped1? If so, we should remember it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the config-hash annotation in the workload will change when migrating to pipedv1. This annotation ensures that the workloads are rolled out when the ConfigMap and/or Secret are changed. The hash calculation will change, but the functionality will not.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! I got it.

Warashi added 2 commits May 8, 2025 13:53
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>
@Warashi Warashi marked this pull request as ready for review May 8, 2025 05:08
Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

LGTM!

@Warashi Warashi enabled auto-merge (squash) May 9, 2025 03:30
@Warashi Warashi merged commit 0ab809a into master May 12, 2025
19 checks passed
@Warashi Warashi deleted the k8s-plugin-manifests-misc branch May 12, 2025 01:06
@github-actions github-actions bot mentioned this pull request May 15, 2025
@github-actions github-actions bot mentioned this pull request May 28, 2025
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.

3 participants