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.

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

Add engine.NewWithClient func #8730

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

object88
Copy link
Contributor

@object88 object88 commented Sep 14, 2020

closes #8725

Signed-off-by: Paul Brousseau object88@gmail.com

What this PR does / why we need it:

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Signed-off-by: Paul Brousseau <object88@gmail.com>
@helm-bot helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 14, 2020
@bacongobbler
Copy link
Member

hi @object88. Generally speaking we ask contributors to provide tests along with their PRs to ensure things are tested.

Have you played around with this PR yet? What do you think about the design? As it stands, right now you have to instantiate the Engine before setting the Strict flag:

e := engine.NewWithClient(client)
e.Strict = true

Perhaps there's a better way to improve the design here. What do you think?

@bacongobbler
Copy link
Member

bacongobbler commented Sep 15, 2020

I'm wondering maybe instead of a NewWithClient, perhaps a new function called func (e *Engine) UseConfig(config *rest.Config) might be slightly better here.

e := Engine{Strict: true, LintMode: true}
e.UseConfig(config)

Happy to hear your thoughts on this one. I don't think we want to make config public after the engine has been instantiated so it cannot be accessed and re-used for another purpose... Seems like poor design for an engine to expose credentials that should not be accessible or usable to the caller.

@object88
Copy link
Contributor Author

Personally, I love using Option funcs pattern, and I see that's in use in the getter package. The engine package doesn't have as many properties to contend with, but I find that it's a good way to ensure that one can expand later. And it allows properties to be kept private (and concurrency-safe, which UseConfig may not, on an assumption).

It wasn't terribly obvious to be what unit tests would be meaningful for the initial PR. But rewriting this with a New(opts ...OptionFunc) to set config, strict, etc., properties would certainly call for it.

@object88 object88 marked this pull request as draft September 21, 2020 19:12
@object88
Copy link
Contributor Author

Sorry for the delays in this; setting to draft while I get to reimplementing.

Signed-off-by: Paul Brousseau <object88@gmail.com>
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 26, 2020
@object88 object88 marked this pull request as ready for review September 26, 2020 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Golang API: how to use engine.RenderWithClient in with Strict enabled
3 participants