Skip to content

Optionally output a unified diff#8

Open
vincentdephily wants to merge 13 commits intocert-manager:mainfrom
vincentdephily:unified_diff
Open

Optionally output a unified diff#8
vincentdephily wants to merge 13 commits intocert-manager:mainfrom
vincentdephily:unified_diff

Conversation

@vincentdephily
Copy link
Copy Markdown

@vincentdephily vincentdephily commented Dec 12, 2025

This PR adds a --patch flag, which outputs a unified diff of the changes needed to fix the boilerplate, instead of just listing the files with issues. So that a user can just run boilersuite --patch . | patch -p0 and then review the changes before committing.

The core changes are done and explained in 486a937. The other commits are refactorings, unittests, error strings, comments, docs, and a bugfix. Hopefully this makes the code more maintainable overall.

@cert-manager-prow cert-manager-prow Bot added the dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. label Dec 12, 2025
@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow Bot commented Dec 12, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sgtcodfish for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 12, 2025
@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow Bot commented Dec 12, 2025

Hi @vincentdephily. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cert-manager-prow cert-manager-prow Bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 12, 2025
@cert-manager-prow cert-manager-prow Bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Dec 22, 2025
@inteon
Copy link
Copy Markdown
Member

inteon commented Feb 11, 2026

@vincentdephily could you split this PR into smaller PRs? Also, @ me to get a faster review.

@cert-manager-prow cert-manager-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2026
@vincentdephily
Copy link
Copy Markdown
Author

vincentdephily commented Feb 13, 2026

Thanks @inteon, I'll rebase and see if I can split this, maybe separate into a unified diff pr and a refactor loading one ? Though the later fix/test commit will have to be reworked.

I also have some ongoing work to make the template list configurable again, a more general/flexible version of what #4 offers, I'll file a separate PR afterwards.

@vincentdephily vincentdephily mentioned this pull request Feb 13, 2026
Open
@vincentdephily
qa: simplify code
83546a8 
There is no need to special-case handling a single file.

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
qa: Delay reading file contents
1b428b6 
This reduces memory use by not reading all files upfront. It should
parallelize better if we later want to do that. It's shorter and IMHO
cleaner.

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
fix: Reenable passing a single file argument
f627035 
Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
qa: Simplify skiplist handling, tweak verbose output
98f0137 
Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
qa: Use plain string instead of regex where appropriate
a1ed959 
Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
feat: Support outputing a patch for failed files
85b351d 
This makes it much easier to fix invalid boilerplate.

It required a significant change to the algorithm, as we need to keep
track of where the existing template is in the file, as well as the
current copyright year. We manually parse comments, but only need to
support the kind seen in boilerplates. We enforce having a newline
before (if applicable) and after the boilerplate, trading flexibility
for consistently and implementation simplicity. I feel the code is
overall cleaner.

This commit implements the base logic, but commits improving the
testing and UX should follow.

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
qa: Refactor template loading and testing
ca77e3a 
* Move functionality from callers and tests into
  `NewBoilerplateTemplate()`.
* Unittest `LoadTemplates()` directly instead of running bitrottable
  checks on template files.
* Simplify code and improve comments a bit.

Short-term, this makes upcoming unittests easier. Longer-term, this
prepares for supporting user-suplied templates, and is IMHO more
maintainable.

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
test: Unittest template parsing, cleanup
be079de 
* Add unittest for template parsing.
* Rename `BoilerplateTemplate` to `Template` and other small
  readability tweaks.

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
test: Unittest Validate() and fix corner cases
382b528 
Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
fix: Use crlf newlines in boilerplate if the file uses crlf
f55d746 
Otherwise the patch would create files with mixed newline
types. Before the `-patch` feature, the problem was sidestepped by
using LF newlines everywhere internally.

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
test: Add TestTemplateFor()
a67394b 
Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
test: Remove smoke_test.sh and fixtures
76bb6b9 
This is now more thoroughly tested using standard go tests.

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@vincentdephily
doc: Update Validation Process to match the new code, and tweak fla…
b0caf36 
…g descriptions

Signed-off-by: Vincent de Phily <vdephily@redhat.com>
@cert-manager-prow cert-manager-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2026
@vincentdephily
Copy link
Copy Markdown
Author

vincentdephily commented Feb 13, 2026

@inteon Here's the reworked branch:

  • Simple refactors have been extracted and moved to Various QA fixes #13 (to be merged first)
  • Some fixup commits have been folded into the parent commit
  • Some commits have been split up for clarity
  • The TemplateFor() refactor has been moved to a future PR

It's still a large patch, but it's a bit more focused. The main commit is the feat:... one, but starting the review from the updated readme (last commit) should help. The other commits are essentially tests, fixes, and readability refactors.

@cert-manager-prow cert-manager-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2026
@cert-manager-prow
Copy link
Copy Markdown
Contributor

cert-manager-prow Bot commented May 5, 2026

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vincentdephily @inteon