Skip to content

Conversation

@porridge
Copy link
Member

No description provided.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.28%. Comparing base (08ab7fb) to head (dec85ba).
Report is 76 commits behind head on main.

Files with missing lines Patch % Lines
pkg/reconciler/reconciler.go 61.53% 4 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (08ab7fb) and HEAD (dec85ba). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (08ab7fb) HEAD (dec85ba)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   85.06%   78.28%   -6.79%     
==========================================
  Files          19       31      +12     
  Lines        1346     2496    +1150     
==========================================
+ Hits         1145     1954     +809     
- Misses        125      451     +326     
- Partials       76       91      +15     

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

Comment on lines +283 to +288
func StripManifestFromStatus(strip bool) Option {
return func(r *Reconciler) error {
r.stripManifestFromStatus = strip
return nil
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Rather than a specific "strip the manifest" manipulation, should we allow arbitrary manipulation of the release that is written to the status? Something like:

func WithReleaseStatusMutator(mutator func(*release.Release)) Option {
	return func(r *Reconciler) error {
		r.releaseStatusMutator = mutator
		return nil
	}
}

@porridge
Copy link
Member Author

porridge commented Mar 19, 2025

Sorry for wasting your time on this, but when looking into changing our code to take advantage of the suggested WithReleaseStatusMutator functionality, I realized we're not using StripManifestFromStatus in the first place 🤦🏻

Looks like at some point we switched to a different mechanism associated with extensions (still to be upstreamed). I'll close this PR and drop this patch from our fork.

@porridge porridge closed this Mar 19, 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.

4 participants