tonobo / hcloud-ruby

Native ruby client for HetznerCloud
MIT License
32 stars 11 forks source link

Label support #43

Closed ghost closed 8 months ago

ghost commented 1 year ago

Implementing label support for different resources.

Core idea of this PR is to add has_labels, which works in the same way as the existing has_actions. When added to a resource class it creates additional methods on this class.

The create methods of resources with label support receive a new parameter labels: {}, setting no labels by default.

To allow to filter resources by labels we add the additional filter attribute :label_selector. This allows users to specify label_selector: 'key=value,key2' to filter. I think that this is enough for now. Providing a Ruby-native way for label selectors would be too much work, because they are quite powerful (label existence, label non-existence, label value equality, label value inequality, AND conjunction, in, notin).

If we need a better way, we could add a LabelSelectorBuilder or similar later. This class could take Ruby-native input and generate label selector strings.

The bulk of changes are test cases.

Example usage:

client = Hcloud::Client.new(token: ENV['HCLOUD_TOKEN'])

# Create an SSH key with labels, one label with a value, one label without
client.ssh_keys.create(
  name: 'keyname', public_key: 'ssh-ed25519 blah', labels: {'creator' => 'username', 'test' => ''}
)

# Update an SSH key's labels; will delete the label "test"
client.ssh_keys["keyname"].update(labels: {'creator' => 'username-new'})

# Pending discussion, see below; add_ and remove_ labels
client.ssh_keys["keyname"].add_labels({'label-without-value' => '', 'one-more-label' => 'some-value'})
client.ssh_keys["keyname"].add_label('label-no-value')
client.ssh_keys["keyname"].add_label('label-with-value', 'my-value')

client.ssh_keys["keyname"].remove_labels(['label-without-value', 'label-with-value'])
client.ssh_keys["keyname"].remove_label('one-more-label')
ghost commented 1 year ago

Open design question @tonobo @RaphaelPour

Should we only support label changes using the update() method? Should we implement a labels= method? Or should we define add_label(s) and remove_label(s) (as proposed in #12)?

My thoughts on this:

The second point for me would indicate that labels= is wrong, because we do not have name= either.

If we want to stay as closely as possible to the Hetzner API, we'd just support update(labels: new_labels) and that's it.

For a user it's probably convenient to be able to add/remove labels to/from the existing set of labels without having to re-calculate the full set of labels all the time. To which I would see two ways to implement, where we'd have to agree on one:

server = client.servers['my-server']
server.name = 'new-name'
server.add_label('foo')
server.add_label('bar', 'baz')
# or then labels = would also work
# server.labels = {'foo': '', 'bar': 'baz'}
server.submit_changes()

The second method (outlined in the code snippet) would also have to incorporate changing the name of resources. Thus - if we decide to go that route - I'd say, let's do it in a separate MR then. If we go that route, I'd only implement update(labels: new_labels) in this MR and we then have enough time to think about this aggregated update mechanism.

ghost commented 1 year ago

At the moment this PR contains two commits:

In label support with add and remove for SSH keys you can see how implementation of label support with add_label(s) and remove_label(s) can work on the example of SSH keys. This is supposed to be an example to show how it would work.

simple label support for all other resource types implements label support in create() and update(), but not the add/remove methods. This is the base functionality that we will need in any case.

After #41 is merged, I'll also add labels to firewalls.

Depending on what we decide to do I will then either remove add_label(s) / remove_label(s) functions from the SSH commit and rebase or add these functions to all other resources and rebase.

ghost commented 1 year ago

Have now created a PR #45 for the base label support for which the implementation is clear. That PR is enough to work with labels.

Then we can use this PR to discuss further user convenience improvements.

ghost commented 8 months ago

Label support is already in hcloud-ruby + this PR has become quite outdated now (cf list of merge conflicts) + currently no need for another label workflow. Closing.