--cert-restart-on-secret-refresh not possible when not using different flag parsing framework

[stijndehaes]

I use a different flag framework to parse flags, and would like this to be a configuration option you pass to the rotator when starting up. However this might be a breaking change for other users. How do you feel about changing this?

[adrianludwin]

What happens if we give the option to set this option when you're starting up the rotator? That way, if you never call flag.Parse(), you get to pick what you want the value to be. But if you do call flag.Parse() and don't pass in the override, then we'll maintain the current behaviour. What do you think?

[stijndehaes]

@adrianludwin that would be fine for me.

So we add the option to the CertRotator struct and in the AddRotator call we do something like:

func AddRotator(mgr manager.Manager, cr *CertRotator) error {
  restartOnSecretRefresh = restartOnSecretRefresh || cr.restartOnSecretRefresh 
  ...
}

I can make a PR for this :)

[adrianludwin]

Yup that lgtm (looks good to me)! Note that I'm not an owner of this project but I suspect that @maxsmythe , @ritazh etc would be happy with this as well.

[maxsmythe]

Not sure I like the idea of mixing the two. Users who do use the standard flag parser may accidentally also set the config option, which would negate the config.

How do people feel about...

1. Starting tagging commits with semver so that we can track non-backwards-compatible changes
2. Removing the flag and implementing a config option, incrementing this semver

Projects using this would then be able to just implement their flag individually should they want to keep it.

I'm not sure how many consumers of this project there are, but hopefully the semver stuff will give them enough warning to avoid surprise.

@ritazh @shomron thoughts?

[adrianludwin]

+100 to semver. I kind of liked "batteries included" but I guess that does make it a bit worse as a library, so I could live with that!

On Thu, Jan 14, 2021 at 4:27 PM Max Smythe [email protected] wrote:

Not sure I like the idea of mixing the two. Users who do use the standard flag parser may accidentally also set the config option, which would negate the config.

How do people feel about...

1. Starting tagging commits with semver so that we can track non-backwards-compatible changes
2. Removing the flag and implementing a config option, incrementing this semver

Projects using this would then be able to just implement their flag individually should they want to keep it.

I'm not sure how many consumers of this project there are, but hopefully the semver stuff will give them enough warning to avoid surprise.

@ritazh https://github.com/ritazh @shomron https://github.com/shomron thoughts?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/open-policy-agent/cert-controller/issues/19#issuecomment-760487372, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE43PZDV4PXMZ4LR7I6S66LSZ5OVRANCNFSM4WAQGP3Q .

[maxsmythe]

Yeah, working with some of these other libraries that use flags... the batteries included approach works great until you need to change the shape of the batteries :p

[adrianludwin]

sgtm

On Thu, Jan 14, 2021 at 8:45 PM Max Smythe [email protected] wrote:

Yeah, working with some of these other libraries that use flags... the batteries included approach works great until you need to change the shape of the batteries :p

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/open-policy-agent/cert-controller/issues/19#issuecomment-760589282, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE43PZAC6MGS7RD4V47BCUDSZ6M2FANCNFSM4WAQGP3Q .

[stijndehaes]

I would be all for doing semver and making this a breaking change in a major release or something similar :)

[ritazh]

+1 on semver and start cutting releases for this project. It would make introducing breaking changes much easier.

[adrianludwin]

@ritazh , @maxsmythe - what do we need to do to make this happen? I'm not an admin on this repo so I can't create a release myself. Can we just call what we currently have "v0.1.0" and then start working towards v0.2.0?

[ritazh]

Since we don't have CI to automate releases yet until https://github.com/open-policy-agent/cert-controller/issues/14 is done, I could manually create/push a tag with the latest commit in master. This should also help unblock https://github.com/open-policy-agent/cert-controller/pull/18 WDYT @maxsmythe?

[maxsmythe]

SGTM

[ritazh]

Sorry for the delay. First release 🎉 https://github.com/open-policy-agent/cert-controller/releases/tag/v0.1.0

[stijndehaes]

I made a PR for making this option part of the CertRotator struct: https://github.com/open-policy-agent/cert-controller/pull/23

[maxsmythe]

Thanks!