--cert-restart-on-secret-refresh not possible when not using different flag parsing framework about cert-controller HOT 15 OPEN

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

from cert-controller.

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?

from cert-controller.

Since we don't have CI to automate releases yet until #14 is done, I could manually create/push a tag with the latest commit in master. This should also help unblock #18 WDYT @maxsmythe?

from cert-controller.

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?

from cert-controller.

@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 :)

from cert-controller.

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.

from cert-controller.

from cert-controller.

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

from cert-controller.

from cert-controller.

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

from cert-controller.

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

from cert-controller.

@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?

from cert-controller.

SGTM

from cert-controller.

I made a PR for making this option part of the CertRotator struct: #23

from cert-controller.

Thanks!

from cert-controller.