Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: dedicated struct for building source data #442

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

refactor: dedicated struct for building source data #442

wants to merge 1 commit into from

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Sep 22, 2024 •
edited
Loading

The main motivation for this PR, is to get closer to a solution to #58, as the bundle controller cannot be used directly for this use case.

But I can identify a few more improvements in this:

  • Better modularization, though far from perfect yet.
  • Avoid calculating the bundle hash for each and every bundle target (which can be many).

@cert-manager-prow
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@cert-manager-prow cert-manager-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Sep 22, 2024
@cert-manager-prow
Copy link
Contributor

[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 ask for approval from erikgb. For more information see the Kubernetes Code Review Process.

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

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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 22, 2024
@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 Sep 25, 2024
@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 Oct 25, 2024
@cert-manager-prow cert-manager-prow bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 9, 2024
@cert-manager-prow cert-manager-prow bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 17, 2024
@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 Nov 22, 2024
@erikgb erikgb changed the title WIP: refactor: dedicated struct for building source data refactor: dedicated struct for building source data Nov 23, 2024
@erikgb erikgb marked this pull request as ready for review November 23, 2024 20:19
@cert-manager-prow cert-manager-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 23, 2024
@erikgb
Copy link
Contributor Author

erikgb commented Nov 23, 2024

/cc @inteon @SgtCoDFish

@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 Jan 10, 2025
@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 Jan 31, 2025
@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 5, 2025
@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 6, 2025
@erikgb
Copy link
Contributor Author

erikgb commented Feb 6, 2025

@SgtCoDFish Would it be possible for you to review this PR? It's been open for "a while", and I am a bit tired of rebasing it. 😅 So I would like to know if this refactoring might be acceptable.

SgtCoDFish
SgtCoDFish requested changes Feb 7, 2025
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

Got a couple of suggestions, what do you think?

pkg/bundle/bundle.go Outdated
@@ -35,51 +34,28 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/cert-manager/trust-manager/cmd/trust-manager/app/options"
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I don't love the idea of depending on ./cmd from ./pkg. I think that'll end in a circular dependency down the road - I'd expect cmd to depend on pkg but not the other way round.

I would vote to keep the options struct where it is in pkg.

(I totally accept that package structuring is a bit arbitrary but I do think it's better for the long term to not risk this kind of circular dependency creeping in)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was actually a circular dependency that made me move Options, but I agree with your comment. Will fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you be ok with moving the options package to ./pkg? I cannot leave the Bundle options where it currently is located under ./pkg - as that will create a circular dependency.

pkg/bundle/internal/target/source.go Outdated
Comment on lines 51 to 77
type BundleBuilder struct {
// a cache-backed Kubernetes Client
Client client.Client

// DefaultPackage holds the loaded 'default' certificate package, if one was specified
// at startup.
DefaultPackage *fspkg.Package

// Options holds options for the Bundle controller.
Options options.Bundle
}

func (b *BundleBuilder) Init() error {
if b.Options.DefaultPackageLocation != "" {
pkg, err := fspkg.LoadPackageFromFile(b.Options.DefaultPackageLocation)
if err != nil {
return fmt.Errorf("must load default package successfully when default package location is set: %w", err)
}

b.DefaultPackage = &pkg

b.Options.Log.Info("successfully loaded default package from filesystem", "id", pkg.StringID(), "path", b.Options.DefaultPackageLocation)
}

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.

suggestion: I think having an Init function introduces the risk of an uninitialized BundleBuilder struct being used somewhere and blowing things up.

A pattern I really like is to have a func NewBundleBuilder(client, defaultPackage, options) (*BundleBuilder, error) - that way, a user can't forget to call Init because creating the object and the init checks are done in NewBundleBuilder.

(That pattern is more useful when the type is unexported because then it's impossible to create a bundleBuilder which hasn't been initialized/validated, but it's not a requirement)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That pattern is more useful when the type is unexported because then it's impossible to create a bundleBuilder which hasn't been initialized/validated

That will require a new interface, right? I am ok with creating a new interface, but are you?

pkg/bundle/bundle.go Outdated Show resolved Hide resolved
@erikgb erikgb marked this pull request as draft February 7, 2025 22:52
@cert-manager-prow cert-manager-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2025
@erikgb erikgb mentioned this pull request Feb 7, 2025
Merged
@erikgb
Copy link
Contributor Author

erikgb commented Feb 10, 2025

/test all

@erikgb
refactor: dedicated struct for building source data
8d44585 
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
@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 13, 2025
@cert-manager-prow
Copy link
Contributor

PR needs rebase.

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

@SgtCoDFish SgtCoDFish SgtCoDFish requested changes

@inteon inteon Awaiting requested review from inteon

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erikgb @SgtCoDFish