Skip to content
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 handling of groups #227

Closed
wants to merge 4 commits into from
Closed

Add handling of groups #227

wants to merge 4 commits into from

Conversation

schmidtandreas
Copy link

This is the implementation of groups handler, that is discussed in #81.

Signed-off-by: Andreas Schmidt <[email protected]>
Signed-off-by: Andreas Schmidt <[email protected]>
@schmidtandreas
Copy link
Author

This is the first try to implement groups handling. Currently the documentation is missing.

@anishathalye
Copy link
Owner

Some high-level thoughts:

When using groups, it seems redundant to specify groups: as the dictionary key at the top level. Right now, Dotbot requires that the config file is a list of tasks. So I think that means that we can implement this functionality in a backwards-compatible way by assigning an interpretation to the top-level being a dictionary, and have it be the group names. So this is basically what you have, except omitting the outer "groups:". Was there a particular reason you had it at the top level?

The overloading of self._only might warrant a bit more thought. Right now, it's used to run a subset of plugins (e.g. just link and not shell). Making it also select groups seems a bit tricky. E.g. what would happen if someone used "link" as a group name? It's probably unlikely that someone would do that, but it seems worth thinking about. Should we add a new CLI flag for selecting groups? (e.g. --groups group [...]) Or is it better to keep the CLI flag set simpler?

@schmidtandreas
Copy link
Author

Was there a particular reason you had it at the top level?
The reason was that I assumed that use only the list / dict to differentiate between groups and tasks could prove inadequate in the future. I think I was too careful and will change it according to your suggestion.

@schmidtandreas
Copy link
Author

schmidtandreas commented Jul 15, 2020

Should we add a new CLI flag for selecting groups? (e.g. --groups group [...]) Or is it better to keep the CLI flag set simpler?
That a good point. If we add two new flags: --group-only group [...] and --group-except group [...]. IMO it would be simple and understandable. A combination with the --only task [...] and / or --except task [...] would then also be possible and would enable better selectability in a clear manner.

@anishathalye
Copy link
Owner

Do you think both only and except for group are necessary? For the plugins I think it's nice to sometimes use except (e.g. to disable the shell plugin, which can sometimes call slow scripts). I'm not sure exactly how people will end up using groups. Would just --group to select a subset of groups be enough?

@schmidtandreas
Copy link
Author

schmidtandreas commented Jul 16, 2020

Do you think both only and except for group are necessary?
Yes, I need the except of group/-s for my tests. My use-case in detail is as follows:

I use Arch-Linux as distribution. Arch-Linux has floating release and the maintainer release every month a new version of installation image. Me and my friend using a installation script arch-install that installs, configs and setup the Arch-Linux corresponding my configuration file.

One step of this script is download, setup and installs the dotfiles using dotbot. Nearly all my dotfiles are public accessible on my dotfile-repo. Except the key-files and credentials-files, they are in a separate git-repo. This private-files-repo is only reachable on my LAN at home, or on the work LAN for the key-files for work-environment. While real installation all necessary repos and thus files are available and reachable.

To determine if something has been changed in Arch-Linux that breaks the installation or setup of the environment, we run the installation script for each configuration file on a VM once a month and check the result. This VM has no access to my private repos, neither on the home network nor on the network at work.

My solution, to avoid false positive while test is to divide the dotfiles in two groups main and keys. While real installation both groups will be processed, but while running the tests I would add --group-except keys to install execution and except the installation of the unreachable key-files. So I would continue to test the installation of most dotfiles and would not get an error due to the inaccessible key files.

That is my use-case for --group-except :) To be honest, I joined conversation #81 to cover exactly this use case.

Or did you mean that is enough to have only --group and I could add all groups that should be processed while the test-run, except the keys in my case? It is also ok for me.

@anishathalye
Copy link
Owner

Also, another thing to consider is whether this needs to be part of the core language, or if the current approach (https://github.com/anishathalye/dotbot/wiki/Tips-and-Tricks#how-can-i-have-different-groups-of-tasks-for-different-hosts-with-different-configurations) is good enough

@schmidtandreas
Copy link
Author

schmidtandreas commented Jul 16, 2020

Yes, that's right, that could also covers my use case. I didn't know it, thanks for the hint! Hmm, then I'm just asking if the groups are still necessary at all. Because the profile files contain nothing but groups. The implementation of the groups would only take it in core, which is nice but not necessary.

@schmidtandreas
Copy link
Author

Maybe you could vote on it? Or you decide that this feature is not essentially necessary. I think it would be an acceptable decision because there is a solution to Issue #81 in terms of groups.

Signed-off-by: Andreas Schmidt <[email protected]>
@schmidtandreas
Copy link
Author

Sorry, to close the discussion was not my intention.

@schmidtandreas
Copy link
Author

I implemented --groups for process only specified groups.

In addition, I tested both variants for my use case. In my view, both solutions are equivalent.
The implementation in dotbot of course offers the comfort that you don't have to change anything in the install script for the use case I have described above. However, I am not sure how many of the dotbot users actually have this or a similar use case. We would therefore add complexity into dotbot that is actually not necessary for most. This has the disadvantages that the dotbot source code becomes a little more complex and thus the maintainability becomes more difficult.
The implementation in install covers the use case just as well. The disadvantage is that every user has to implement and maintain it themselves.
I would prefer the script solution for my use case. However, there may be other use cases where the implementation in dotbot is better. So I stick to my statement beforehand: "I rely on your decision as maintainer" .
If you decide to take over, I would expand the documentation accordingly. If not, let me know and I'll close this PR.

@anishathalye
Copy link
Owner

Thank you for taking the time to iterate on this. It's a difficult decision, but for now, I think it's better to stick with the script solution.

I thought of another complexity related to this being implemented in Dotbot: once the top-level becomes a dictionary rather than an array, ordering is no longer guaranteed. Dotbot could end up executing the groups of commands "out of order" with respect to how they appear in the file, which could confuse users. We could work around this with a slightly hacky approach of having PyYAML read into an OrderedDict, but that doesn't seem ideal; we'd need an analogous hack for JSON, and having a nonstandard interpretation of YAML/JSON (with dictionaries being ordered) seems non-ideal. Seems like it's a limitation of implementing a DSL in YAML, that as it gets more complicated, things can get messy/confusing...

As I mentioned over in another issue (#233 (comment)), I'm going to try to spend some time thinking over all the Dotbot issues, see how people use Dotbot in the wild, and see if we can come up with some clean solutions to these problems without having to worry about backwards compatibility. I'll make sure to think about groups of tasks while doing that; and this iteration and discussion has definitely been helpful to my understanding of the issue, so thank you for that :)

@schmidtandreas
Copy link
Author

Thank you for your time, it also helped me a lot to understand dotbot better and to use it more effectively.

@schmidtandreas schmidtandreas deleted the add_groups_handler branch August 9, 2020 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants