Skip to content

Conversation

sethiyash
Copy link
Contributor

What this PR does / why we need it:

Which issue(s) this PR fixes:

#1494

Fixes #

Does this PR introduce a user-facing change?


Additional Notes for your reviewer:

Review Checklist:
  • Follows the developer guidelines
  • Relevant tests are added or updated
  • Relevant docs in this repo added or updated
  • Relevant carvel.dev docs added or updated in a separate PR and there's
    a link to that PR
  • Code is at least as readable and maintainable as it was before this
    change

Additional documentation e.g., Proposal, usage docs, etc.:


Signed-off-by: Yash Sethiya <yashsethiya97@gmail.com>
@sethiyash sethiyash force-pushed the 1494-app-factory-interface branch from eaa0ae7 to d46919f Compare February 26, 2024 10:08
)

// ICRDAppFactory interface for CRDAppFactory
type ICRDAppFactory interface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ICRDAppFactory Maybe not a good name let me know if you have some better suggestions.

@sethiyash
Copy link
Contributor Author

@jorgemoralespou, Is this what you are looking for?

@jorgemoralespou
Copy link

Yes. Thank you for getting to this so fast.

@renuy renuy requested review from prembhaskal and 100mik March 5, 2024 06:14
@prembhaskal
Copy link
Member

LGTM

Comment on lines +24 to +27
// ICRDAppFactory interface for CRDAppFactory
type ICRDAppFactory interface {
NewCRDApp(*kcv1alpha1.App, logr.Logger) *CRDApp
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// ICRDAppFactory interface for CRDAppFactory
type ICRDAppFactory interface {
NewCRDApp(*kcv1alpha1.App, logr.Logger) *CRDApp
}
// CRDAppCreator interface for CRDAppFactory
type CRDAppCreator interface {
Create(*kcv1alpha1.App, logr.Logger) *CRDApp
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants