-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new interface to register experiments #5755
Conversation
7c329c0
to
3116e29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this change! I find it much easier to reason about and I'm guessing it's less bug-prone than the previous use of options
being passed around everywhere 🙌
$options[:updater_options].each do |name, val| | ||
Dependabot::Experiment.register(name, val) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use :updater_options
for more than experiments? Maybe it's not such a big deal for dry-run
, but the name doesn't really fit the purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we use a few different names in different places, we ended up with an options
arg for the FileUpdater
with the idea that it could be used for more things than just experiments. I'm not sure that we ever have, and going forward we should probably rename things where it makes sense, but I was aiming to keep the change as small as possible
Currently experiments are required to be passed in as an optional `options` flag to the individual classes like the FileFetcher and FileUpdater. This is fine, but it becomes a bit cumbersome when more deeply nested objects need to access this flag, as we need to pass it down to all of them. This introduces a global `Dependabot::Experiments` interface that we can use to register experiments and access them where needed. The idea is that these are set in the Updater before running an update, and will be available for the duration of the job.
3116e29
to
aa2bb20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels much cleaner than before. 🚀
cc @brendandburns this change affects how the |
Thanks @jurre, funny having never looked at the actual code, I'd always assume feature/experiment flags were implemented this way already. 😆 Is there any cleanup that needs to be done in a follow-on PR to remove code / feautres that this PR makes superfluous? Based on the conversation ☝️, I wasn't clear if the |
Yeah we can migrate the existing flags to use this functionality, but we can also keep them as is and wait for them to naturally be removed as those experiments are wrapped up. |
Currently experiments are required to be passed in as an optional
options
flag to the individual classes like the FileFetcher andFileUpdater. This is fine, but it becomes a bit cumbersome when more
deeply nested objects need to access this flag, as we need to pass it
down to all of them.
This introduces a global
Dependabot::Experiments
interface that we canuse to register experiments and access them where needed.
The idea is that these are set in the Updater before running an update,
and will be available for the duration of the job.
Right now most of the existing experiments are left as is, using the
options
flags,but I've ported the
kubernetes_enabled
feature flag for the Docker ecosystem todemonstrate how this would work.
There is more functionality we could add to this interface, but for now it just adds what
is needed.