Handle gittracks flag #171
Conversation
DanielMorsing
force-pushed
the
disallow-gt-to-cgto
branch
from
80da0ab
to
b50631f
Compare
DanielMorsing
force-pushed
the
handleGittracks-flag
branch
from
a7e92c8
to
077cd8b
Compare
DanielMorsing
force-pushed
the
handleGittracks-flag
branch
3 times, most recently
from
45acc5e
to
e9cd755
Compare
pkg/controller/gittrack/handler.go
Outdated
| // don't reconcile if it's a gittrack and they're disabled | ||
| // we shouldn't ever get here because reconcile should be turned off at the controller level, | ||
| // but for testing and safeguarding, check anyway | ||
| if _, ok := gt.(*farosv1alpha1.ClusterGitTrack); ok && r.gitTrackMode == farosflags.GTMDisabled { |
There was a problem hiding this comment.
I don't think we should perform this evaluation, given it is going to happen for every reconcile and we don't have it's counterpart for ClusterGitTracks, we should just test and rely on the controller level off switches
There was a problem hiding this comment.
On the grand scheme of things, this evaluation doesn't cost anything and it only makes the code more robust.
The controller level watches are being turned off, so it's not a case of checking on every reconcile. Since reconcile being called on a type with the mode disabled is a logic error, I'm going to change this into being a panic
| @@ -57,6 +60,7 @@ func init() { | |||
| FlagSet.BoolVar(&ServerDryRun, "server-dry-run", true, "Enable/Disable server side dry run before updating resources") | |||
| FlagSet.DurationVar(&FetchTimeout, "fetch-timeout", 30*time.Second, "Timeout in seconds for fetching changes from repositories") | |||
| FlagSet.StringVar(&RepositoryDir, "repository-dir", "", "Directory in which to clone repositories. Defaults to cloning in memory if unset.") | |||
| FlagSet.Var(&GitTrack, "gittrack-mode", "Whether to manage GitTracks. Valid values are Disabled and Enabled") | |||
There was a problem hiding this comment.
Is it possible to set defaults? We should have a default for cluster-gittrack-mode too preferably
There was a problem hiding this comment.
The default is already set by virtue of the zero-value being false
There was a problem hiding this comment.
Is there any way to be more explicit? That's quite "magic" and kinda not obvious IMO. What's the default value for the other flag, clustergittrack-mode?
DanielMorsing
force-pushed
the
handleGittracks-flag
branch
from
dc337ff
to
221db4f
Compare
DanielMorsing
force-pushed
the
disallow-gt-to-cgto
branch
from
aefbc72
to
0d25e8c
Compare
Also remove a test that checked on running disabled CGT, since it is now a panic
DanielMorsing
force-pushed
the
handleGittracks-flag
branch
from
221db4f
to
0aca915
Compare
We didn't catch this, because we set this value explicitly in the tests
|
/retest |
Final piece: Add a flag for disallowing (regular) GitTracks