-
Notifications
You must be signed in to change notification settings - Fork 413
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
Changelog entry for the change of dune cache default (#10710) #10913
Conversation
A couple of comments:
|
dee6b50
to
4e1af03
Compare
Thanks for the update @ElectreAAS ! I have a last question, is it possible with the new code to recover the old behavior when the user had used |
Not all rules are cached by default: user-created ``(rule _)`` actions are not to prevent mistakes. | ||
You can enable the caching of these rules by setting the environment variable ``DUNE_CACHE_RULES`` to ``enabled``. | ||
|
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.
to be clear, this is about caching the output of such rules right? Not the rules themselves
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 don't really know what caching a rule without caching its result would even mean. So yes I believe this is about caching outputs.
No, not with the current code, you need to explicitly opt-in to caching the user rules. |
As Marek mentioned, it's not possible at the moment. If you'd like to add a config option for this in addition to the env var, you are welcome to. |
Thanks folks for the confirmation. I think indeed an option to enable this in I guess that would be a regression, but likely this setup did fall under the "experimental" category, right? |
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Signed-off-by: Ambre Austen Suhamy <[email protected]>
Signed-off-by: Ambre Austen Suhamy <[email protected]>
426c993
to
521ca04
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 am aware of the discussion happening in #10710 but I think this PR accurately describes the current state of main
. If any changes will be done that will happen in a separate PR so I think merging this one in the meantime is sensible.
Signed-off-by: Christine Rose <[email protected]>
The discussion in #10710 is converging, so you may want to hold off before merging this one. See #10944. |
#10944 has been merged, and it includes a correct changelog. |
No description provided.