Skip to content

Fixed to add annotations after calling loader.LoadManifests #5777

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 1 commit into from
Apr 30, 2025

Conversation

ffjlabo
Copy link
Member

@ffjlabo ffjlabo commented Apr 24, 2025

What this PR does:

Move the logic to add the annotation logic into the plugin side.

Why we need it:

When creating the commit, the fixes are not related to the target app, there is a diff for pipecd.dev/commit-hash.
The current logic detects diffs that are defined in the git repo but do not exist in the actual resource.

So, IMO, we shouldn't include them when loading manifests from the git repo because these annotations are set automatically before applying resources on the piped side.

PipeCD

Which issue(s) this PR fixes:
Part of #5006 #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):

Copy link

codecov bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 89.47368% with 4 lines in your changes missing coverage. Please review.

Project coverage is 27.14%. Comparing base (61169c4) to head (b55e6cb).
Report is 7 commits behind head on master.

Files with missing lines Patch % Lines
...app/pipedv1/plugin/kubernetes/provider/manifest.go 0.00% 2 Missing ⚠️
...lugin/kubernetes_multicluster/provider/manifest.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5777      +/-   ##
==========================================
+ Coverage   27.08%   27.14%   +0.05%     
==========================================
  Files         504      504              
  Lines       53187    53195       +8     
==========================================
+ Hits        14405    14439      +34     
+ Misses      37708    37682      -26     
  Partials     1074     1074              

☔ 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.

@ffjlabo ffjlabo marked this pull request as ready for review April 25, 2025 04:03
@ffjlabo ffjlabo enabled auto-merge (squash) April 25, 2025 05:56
Copy link
Contributor

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

👍🏻

@ffjlabo ffjlabo merged commit ada6f25 into master Apr 30, 2025
19 checks passed
@ffjlabo ffjlabo deleted the add-annotations-after-loading-manifests branch April 30, 2025 02:34
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