Reformulate --registry-scanning

[squaremo]

The flag --registry-scanning is well-motivated, but can be refined further.

1. It should be --disable-registry-scanning (or --registry-disable-scanning)

In general, boolean flags that default to true should be used sparingly if at all. Such a flag has to be given a value to have an effect, which breaks the expectation that a boolean flag is provided without a value. (And there are other problems with default-true boolean values; for example, if you have a go struct defining the config, the field value will default to false rather than true, so you have to be extra careful about how you populate it).

You can argue that it's awkward to have a --disable-X flag -- yes it is. Designing from scratch, you would have --enable-registry-scanning to switch that particular optional thing on. But the starting point is that fluxd already does a bunch of things, so you have to go the other way around, and switch things off -- and the less surprising way to do it is to use flags which default to false.

2. I don't think --registry-scanning=false should imply --git-readonly.

It doesn't logically imply it -- it's totally possible to use a writable git repo while switching registry scanning off.

But there's no particular motivation for making git read-only when you're not scanning image registries, either. Automation of image updates isn't possible if you are not looking at image metadata, but that doesn't mean you don't want to write to the git repo -- you may still use a tag as the high-water mark, and you may still want to be able to e.g., change automation policies.

A better factoring would be to have --registry-scanning=false imply disabling automation, whether or not that's controlled by a switch itself.

[2opremio]

No strong opinions on (1)

About (2), I am actually with you but @stefanprodan pushed for that arguing user expectations (and he may be right), so I will let you two sort that one out :)

[stefanprodan]

I don't think --registry-scanning=false should imply --git-readonly

It's the other way around, git-readonly=true implies registry-scanning=false
https://github.com/fluxcd/flux/blob/master/cmd/fluxd/main.go#L280

[squaremo]

https://github.com/fluxcd/flux/blob/master/cmd/fluxd/main.go#L154

[stefanprodan]

Ok so we need to reformulate that to git-readonly=true implies registry-scanning=false

[squaremo]

I'm not sure I agree with the "usage implication" running the other way, either: it is possible you might want to be able to query for the image metadata, but not run automation.

From the point of view of "what's the easiest way to access the common modes of use", roughly we have these modes of use:

1. full featured with automation (i.e., don't disable anything)
2. just sync from git, don't try to update anything
3. sync from git and give me a view of what's running (e.g., connect to Weave Cloud)

Having --git-readonly imply --registry-scanning=false would work if it were only the first two, since --git-readonly would effectively stand in for "sync only mode". But because of the last mode of use, it's desirable to be able to make git readonly without also removing image metadata. The trade-off is that you need to supply two flags if you want to run in mode 2.).

[2opremio]

Ok so we need to reformulate that to git-readonly=true implies registry-scanning=false

True, the code does the right thing though

[squaremo]

Ok so we need to reformulate that to git-readonly=true implies registry-scanning=false

True, the code does the right thing though

Yes, there is the different but also serious problem of the argument not behaving as the usage message say it does :-/

[2opremio]

Yes, there is the different but also serious problem of the argument not behaving as the usage message say it does :-/

Of course, I will fix it in one way or another based on what's decided here.

[squaremo]

@stefanprodan Do you buy the argument in terms of modes of use, made above?

[stefanprodan]

People that I've talked to on Slack and elsewhere wanted the readonly mode to allow them to run Flux stand-alone without memcached. I don't see why would someone enable readonly mode and have a need for Flux to scan the registries. While your point is valid, I've never seen a user asking for readonly without asking how to disable registry scanning also. As for Weave Cloud, I don't think we allow users to switch to readonly.

Having said that, I'm not going to insist on this, if you think it's best to use two flags, we can undo the change and document how to make Flux truly readonly with --git-readonly=true and --registry-scanning=false or --registry-disable-scanning=true.

[2opremio]

No strong opinions on my side. @squaremo make a call and I will make adjustments one way or another :)

[hiddeco]

Based on messages I have received from the community I have to side with @stefanprodan, but I'll leave it up to you two to make the final call.

[2opremio]

I think we have a compelling usecase to keep --git-readonly and --registry-scanning independent: the ability to keep using the git tag to keep track of syncs while disabling repo scanning. See #2745 (comment)

So, now I am with @squaremo .

[2opremio]

I would keep the name of the flag as is though.

[2opremio]

I think we have a compelling usecase to keep --git-readonly and --registry-scanning independent: the ability to keep using the git tag to keep track of syncs while disabling repo scanning. See #2745 (comment)

Actually, how it is now --git-readonly=true implies --registry-scanning=false and not the other way around (although the help message is wrong). So, the user can still freely do --registry-scanning=false and --git-readonly=false.

[2opremio]

Anyways. I think I will keep the flags independent for now.