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

Tags #125

Merged
merged 6 commits into from
Sep 8, 2020
Merged

Tags #125

merged 6 commits into from
Sep 8, 2020

Conversation

adamtulinius
Copy link
Contributor

Background:

I have multiple deployments that are pretty much fully automated, except that the hosts must be updated in an order different to the default lexical ordering. This is due to the deployment containing different types of hosts, such as master and slave hosts.
Furthermore, it would be nice to have an easy way of selecting a specific type of host, without having to resort to matching on host names.

This PR implements

  • a way of adding tags to hosts (deployment.tags)
  • using these tags to filter what hosts to work on (--tagged foo,bar
  • ordering the hosts by their tags (--order-by-tags=bar,foo), and
  • a default ordering can be specified using network.ordering.tags

New arguments:

  • --tagged="" Select hosts with these tags
  • --order-by-tags="" Order hosts by tags (comma separated list)

New attributes:

  • network.ordering.tags = [ "foo" "bar" ]
  • `deployment.tags = [ "foo" "bar"]

Naming things are always difficult, but i figured

  • morph build deployment.nix --on "..." --tagged "..." sounded pretty decent, and
  • --order-by-tags instead of --order-by gives us room to implement multiple different orderings that can maybe be combined

Cheers

@@ -73,6 +73,11 @@ rec {

machineList = (map (key: getAttr key machines) (attrNames machines));
network = network'.network or {};
deployment = {
hosts = machineList;
meta = filterAttrs (n: _: n != "pkgs") network;
Copy link
Contributor

Choose a reason for hiding this comment

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

This disallows custom attrs that aren't serializable to JSON, e.g. functions. Some users might depend on being able to stick whatever in network. It also breaks stuff for us; for example: Try building k8s-prod.nix in our deployments repo.

Instead of filtering out "pkgs", I suggest inverting the filter and only selecting the attrs morph knows about.

Other than this comment, the rest of the PR looks good to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. ordering and description attrs are now the only selected ones.

@johanot
Copy link
Contributor

johanot commented Sep 8, 2020

Oh... Almost forgot.. Can you please document this feature somewhere in the README as well?

@adamtulinius
Copy link
Contributor Author

Oh... Almost forgot.. Can you please document this feature somewhere in the README as well?

Done. I also added tags to examples/simple.nix, and while doing so bumped nixpkgs in each example, since they didn't actually build anymore.

@adamtulinius
Copy link
Contributor Author

Now I truly am done changing things around. I hope. ;)

I changed the behavior of --tagged to require all listed tags, since that seemed more useful after a bit of experimentation. If we need "or" functionality, we can make --tagged repeatable or something, or go all the way and provide more intelligent filtering.

@johanot
Copy link
Contributor

johanot commented Sep 8, 2020

I've looked through the recent changes and I'm fine with the current implementation as is. There are many additional improvements and additions one could imagine, but I'm also happy with the simplicity of this implementation.

In the future, I think I would be fan of a lambda-filter of sorts, similar to that of --build-target. Something like --filter '(h: h.config.services.nginx.enable && h.config.deployment.tags == [ "web" ])' - that would provide ultimate flexibility. For simple usecases though, I think --tagged ... is way more ergonomic, hence both features would probably be able to co-exist.

@johanot johanot merged commit c048d63 into master Sep 8, 2020
@adamtulinius adamtulinius deleted the tags branch September 8, 2020 14:24
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.

None yet

2 participants