add option to set load_restrictor to none #101
Conversation
|
What a great looking PR, 12/10 would merge 😉 |
| @@ -17,6 +18,15 @@ func dataSourceKustomization() *schema.Resource { | |||
| Type: schema.TypeString, | |||
| Required: true, | |||
| }, | |||
| "load_restrictor": &schema.Schema{ | |||
There was a problem hiding this comment.
In the schema, even if for now we only implement the loadrestrictions, I think we should still wrap it in a hash map. Because I'm sure somebody will eventually have a requirement for another one of the supported options.
And I think it would be cleaner to not add all of them to the schema at the top level? What do you think about that @darren-reddick?
There was a problem hiding this comment.
In fact there was already a question about making the plugins configurable, which I suppose could also go into that hash map. I'm not saying you need to implement that as well. Just saying, wrap the load_restrictions into a hash map, and maybe it will be extended in the future.
There was a problem hiding this comment.
OK. Makes sense - just re-reading the issue and see that you hinted at this. I'll update this.
|
Thanks for the contribution Darren. Only small change I think would be good is the schema change. Happy to discuss. |
darren-reddick
force-pushed
the
feature/no-load-restrictor
branch
from
e12adaf to
f5c279d
Compare
|
Hi, |
| func kustomizationBuild(d *schema.ResourceData, m interface{}) error { | ||
| path := d.Get("path").(string) | ||
|
|
||
| kOpts := getKustomizeOptions(d) |
There was a problem hiding this comment.
Wouldn't it be better to pass the entire kOpts into runKustomizeBuild and handle the defaults and overwrites in there just once, instead of handling it once for kustomization_build and once for kustomization_overlay?
There was a problem hiding this comment.
Hi. I'm not sure I follow this here. Do you mean to remove the code for setting the default and checking the load_restictor from getKustomizeOptions down into runKustomizeBuild?
There was a problem hiding this comment.
Originally, you changed runKustomizeBuild to accept an additional parameter load_restrictor string and passed that string in right from the schema. But now, we're wrapping the load_restrictor in a hash map. So you added getKustomizeOptions to get the string form the hash map and be able to pass it into the runKustomizeBuild method. But inside that method is where it current gets the default config, and if load_restrictor was not none, it overwrites that value.
I was wondering, if it wasn't cleaner to pass the hash map into runKustomizeBuild, the same way it comes from Terraform, and then handle conversion, defaults and overwriting all just in one place. You wouldn't have to call getKustomizeOptions from two places. Plus getKustomizeOptions could also handle the defaults and overwriting, that's now implemented in runKustomizeBuild.
Basically I'm asking if we should not replace:
opts := krusty.MakeDefaultOptions()
// If the load_restrictor option is set to none then the krusty.Options
// should be updated to reflect this.
if load_restrictor == "none" {
opts.LoadRestrictions = types.LoadRestrictionsNone
}
with
opts := getKustomizeOptions(kOpts)
and change runKustomizeBuild to accept kOpts *schema.ResourceData
runKustomizeBuild(fSys filesys.FileSystem, path string, kOpts *schema.ResourceData)
There was a problem hiding this comment.
But I'm fine merging without that change too. Since all these methods are private, I can probably change this at any time in the future.
There was a problem hiding this comment.
ok. Passing the schema thru was what i thought you meant originally but then had doubts. That totally makes sense so I will update.
There was a problem hiding this comment.
OK. Have updated this and its much cleaner now. Having the new kustomizeOptions type didnt make any sense also so have removed that.
| @@ -17,6 +17,18 @@ func dataSourceKustomization() *schema.Resource { | |||
| Type: schema.TypeString, | |||
| Required: true, | |||
| }, | |||
| "kustomize_options": &schema.Schema{ | |||
| loadRestrictor string | ||
| } | ||
|
|
||
| func getKustomizeOptions(d *schema.ResourceData) (k kustomizeOptions) { |
There was a problem hiding this comment.
This function is probably better located in the same file as runKustomizeBuild.
|
Thanks Darren, this looks great. I'll get it merged to run the tests. I should probably run the tests on PRs not just on commits. To get them run on contributions like this. |
PR kbst#101 added the ability to set load restrictor to `"none"` This adds documentation for that change
Hey!
Hope you are good.
This PR is to add support for
--load_restrictor=noneas requested in issue #70 .I have tested the functionality locally but can look at pushing an update to the acceptance tests to specifically test this if needed.
Cheers,
Darren