Validate resource names for drift remediation flags

[a-hilaly]

[fixes https://github.com/aws-controllers-k8s/community/issues/1647]

As discussed in #106

this patch brings resource name validation to the arguments passed to
--reconcile-resource-resync-seconds. It also slightly changes the
previously implemented ParseReconcileResourceResyncSeconds to avoid
uncessary validation ops.

Signed-off-by: Amine Hilaly hilalyamine@gmail.com

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[jaypipes]

I'm good with this. In the future, it may be good to adopt an "option strategy" for the Config validation (or initialization). This "option strategy" would allow for introducing changes to the runtime that would not break binary/compile-time compatibility in the code generator (like has happened on the related PR there that introduces the resource manager factories getter).

The "option strategy" would look something like this:

// pkg/config/option.go
type Option struct {
    kinds []string
    // other fields that can be used in validation/initialization...
}

// WithKinds instructs the configuration to validate against a set of
// supplied resource kinds
func WithKinds(kinds []string) Option {
    return Option{kinds: kinds}
}

// mergeOptions merges all Option structs into a single Option
// and sets any defaults to missing values
func mergeOptions(opts []Option) Option {
    merged := Option{}
    for _, opt := range opts {
        if opt.kinds != nil {
            merged.kinds = opt.kinds
        }
        // repeat for each field in Option ...
    }
    // set any defaults on merged if still nil...
    return merged
}

// pkg/config/config.go
...
// Note this could also be a New() function instead of Config.Validate()...
func (cfg *Config) Validate(
    opts ...Option,
) error {
    merged := mergeOptions(opts)
    if merged.kinds != nil {
        // Validate the kinds...
    }
}

This way as we add additional validations or configuration options, we don't need to change the function signature for Config.Validate() -- or pkg/config.New().

Your code-generator template code would call like so:

if !cfg.Validate(WithKinds(resourceKinds)) {
     // blah blah..
}

[jaypipes]

you can just name this alias ackutil since we're already in the runtime package.

[a-hilaly]

I'm good with this. In the future, it may be good to adopt an "option strategy" for the Config validation (or initialization). This "option strategy" would allow for introducing changes to the runtime that would not break binary/compile-time compatibility in the code generator (like has happened on the related PR there that introduces the resource manager factories getter).

I love this idea. Let me implement it in this PR...

[jaypipes]

muy bueno, thanks @a-hilaly!

[RedbackThomson]

I don't really understand the value in the options struct? I think it's an early optimisation that isn't adding much - especially since we aren't providing defaults.

[a-hilaly]

It's a know paradigm in Go - we could definitely do a follow up to bring default values this way. I agree that it's not bringing that much, but it allows us to add good stuff if needed :)

[a-hilaly]

Thinking about https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/config/config.go#L253-L255

[RedbackThomson]

/lgtm

[ack-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [A-Hilaly,RedbackThomson,jaypipes]

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