Skip to content

Add immutableByDefault option #225

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

Merged
merged 4 commits into from
Jun 25, 2024
Merged

Add immutableByDefault option #225

merged 4 commits into from
Jun 25, 2024

Conversation

magnouvean
Copy link
Collaborator

This adds support for automatically marking all keys written by plasma-manager as immutable by default, including keys written by higher level modules. For this to be possible, immutability (and shellExpansion even though not strictly needed) and persistence have now become allowed for the same key. It's worth noting that immutability will in these cases not have any meaning, so for example:

group.key = {immutable = true; persistent = true;};

will result in the key key in group group being persistent, not key[$i]. I'm not completely settled on this choice, but it comes from the need of marking non-immutable keys as persistent as well. Typically the keys we want to mark as persistent are not immutable so it makes sense in this way (we basically never have to make immutable keys persistent). I would love other's thoughts on this as well. I have not done much testing, but it seems to work fine for me. Probably needs a little more testing to make sure everything works before I'll merge @Asqiir.
Closes #217.

Copy link
Contributor

@Asqiir Asqiir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this might bring us a lot of users later on that ask us "why does immutability not work??"

For the check that immutability and persistence aren't enabled at once: maybe only disable that check when immutabilityByDefault is enabled.

I believe that there might be a more elegant solution to achieve this, but this is already pretty good and pretty quick :)

@magnouvean
Copy link
Collaborator Author

I like the suggestion, though the code implementation wouldn't be particularly elegant as we'd need to pass in extra arguments to the config-writing script and pass this into a lot of functions, which would clutter the code quite a bit. We can see if we get more bug reports following this change whether it's worth implementing, though it would only be applicable to cases where the user has enabled peristency manually together with immutability, which probably is quite niche of a use-case. Additionally, at least to me it doesn't really make sense that group.key = { persistent = true; immutable = true; }; should make key[$i] persistent, so I kind of think the present solution makes just as much sense.
Great that it works! All the tests I've done have looked good, so I can probably merge rather soon :)

@magnouvean
Copy link
Collaborator Author

magnouvean commented Jun 23, 2024

Actually thinking about it, maybe it makes sense to check if the immutable setting is simply different from the default (so it'd error for persistent keys for non-immutable keys when immutableByDefault is enabled and for immutable keys when immutableByDefault is disabled, just like before). That I think is good enough of a solution to be worth the slight complication.

@magnouvean
Copy link
Collaborator Author

I managed to implement this just fine I think. Will test some more and hopefully get this merged in a couple days.

@magnouvean magnouvean merged commit a3b881f into nix-community:trunk Jun 25, 2024
1 check passed
Asqiir pushed a commit to Asqiir/plasma-manager that referenced this pull request Jun 26, 2024
toast003 pushed a commit to toast003/plasma-manager that referenced this pull request Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option everythingImmutable or immutableByDefault
2 participants