Update --preflight command line option #894
Conversation
| // Returns an error if there is a problem | ||
| // parsing the preflight checks | ||
| func (c *Registry) Set(s string) error { | ||
| if c.known == nil { | ||
| return nil | ||
| } | ||
|
|
||
| enabled := map[string]struct{}{} |
There was a problem hiding this comment.
Non blocking, but using the sets package would make it so this can be a new sets.Set instead of doing this map[string]struct{}{} setup. Then you can do things like sets.Insert(enabled, "key") and enabled.Has("key")
There was a problem hiding this comment.
In this case it was a temporary storage, avoid disabling everything, then enabling only things we cared about. We don't actually care about the the value, just which one's have been enabled.
I suppose, if a Set were used, we could set the Set to disabled, then run through the passed in commands, and go through the Set and enable/disable...
There are so many ways to do this!
There was a problem hiding this comment.
A sets.Set is used exactly for the purpose of only caring about a key without the value. It's a generic map to an empty struct:
type Set[T comparable] map[T]Empty
Again, non-blocking because like you said there are many ways to do this but this is a nice helper for this that is in the kube apimachinery package :)
There was a problem hiding this comment.
[thought] Would have been our first use of generics 😆 , but nothing to add on here. Sounds good to me either way!
I was looking at your original PR, and somehow missed you had updated e2e... I thought my local e2e failures were due to my environment... will update. |
Fix carvel-dev#893 The `--preflight` command line options now only specifies those preflight checks that are to be enables. Signed-off-by: Todd Short <[email protected]>
|
Doc PR: carvel-dev/carvel#733 |
|
@everettraven I am curious about how the flag would affect the proposed way of configuring preflights here, but we can discuss that further in the proposal itself |
We decided to move preflight configuration into the configuration file (rather than be part of the command line flags). That PR should be updated now to reflect that. |
|
ping for approval? |
|
@praveenrewar merge away if this looks good to you, I am good with these changes. Let's explore where we are headed further in the proposal 🙌🏼 |
Fix #893
The
--preflightcommand line options now only specifies those preflight checks that are to be enables.What this PR does / why we need it:
This simplifies the command line
--preflightoption; we're looking to have additional configuration located in a configuration file, rather than on the command line.Which issue(s) this PR fixes:
Fixes #893
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: