Skip to content

valkey: add argument_specs, move the provider check out of the task - #719

Open
GeertJohan wants to merge 1 commit into
metal-stack:masterfrom
GeertJohan:argument-specs/valkey
Open

valkey: add argument_specs, move the provider check out of the task#719
GeertJohan wants to merge 1 commit into
metal-stack:masterfrom
GeertJohan:argument-specs/valkey

Conversation

@GeertJohan

@GeertJohan GeertJohan commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Split out of #707, which was too large to review in one go.

This is the same change for a single role, so the pattern can be judged on something small. Picked valkey because it's deployed by mini-lab.

What it does

  • adds control-plane/roles/valkey/meta/argument_specs.yaml, documenting every variable the role takes (type, description, no_log on the secrets)
  • moves valkey_backup_restore_sidecar_provider in ["local", "gcp", "s3"] int the spec as choices
  • drops the second assert block, which re-checked valkey_backup_restore_sidecar_image_tag is defined - the first assert already covers that variable

Why the image variables stay in the assert

valkey_image_name/_tag and valkey_backup_restore_sidecar_image_name/_tag are resolved from the release vector by this role's own setup_yaml task.
Ansible inserts role argument validation before the first task of the role, so those variables are not defined yet at validation time and marking them required: true would fail every run. They are declared in the spec (for types and ansible-doc) but stay in the assert for the presence check. There is a comment in the spec saying so, so the next reader does not "fix" it.

Effect

A wrong ..._provider value now fails before any task runs instead of at the assert, and ansible-doc -t role can list the variables.

If this looks right, I will send the remaining roles the same way, a few per PR.

Used AI? ✨

Yes. Claude Fable 5

@GeertJohan
GeertJohan requested a review from a team as a code owner August 11, 2026 14:38
@GeertJohan
GeertJohan requested a review from vknabel August 11, 2026 14:38
@metal-robot metal-robot Bot added the area: deployment Affects the deployment area. label Aug 11, 2026
@Gerrit91
Gerrit91 self-requested a review August 11, 2026 14:56
@Gerrit91

Copy link
Copy Markdown
Contributor

/ok-to-build

@metal-robot metal-robot Bot mentioned this pull request Aug 17, 2026

@Gerrit91 Gerrit91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finally found some time to look at it and I really like it!

The role already is a good example that the documentation is outdated. Do you think it would be possible to generate the role's README.md from ansible-doc -t role -r control-plane/roles valkey --json with a small python script helper?

I think it would work if:

  • The role description would also be moved into the argument spec
  • We come up with some generic doc snippets to be added in case the role depends on setup_yaml (with a link to how resolving the release vector works?) or when shared default variables are imported through role dependencies
  • To enable everyone to update those docs, the generation could be invoked through a small container (similar to what I tried here: metal-stack/metal-ansible-modules#30)
  • Did you consider adding the defaults to the argument spec? It seems pretty hard to keep them in-sync with the role defaults but would be really cool in a table.

Comment on lines +88 to +90
valkey_registry_auth_enabled:
type: raw
description: Whether to deploy an image pull secret for the registry.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Must be a bool

@GeertJohan

Copy link
Copy Markdown
Contributor Author

I like the idea of having README.md's generated from spec, would it make sense to have that be a separate PR to avoid scope creep?

Documents every variable of the valkey role in meta/argument_specs.yaml,
so ansible validates types before the role runs and `ansible-doc -t role`
can show them.

Two checks leave tasks/main.yaml:

- `valkey_backup_restore_sidecar_provider in ["local", "gcp", "s3"]`
  becomes `choices` in the spec.
- the second assert block was a duplicate: it re-checked
  `valkey_backup_restore_sidecar_image_tag is defined`, which the first
  assert already covers.

The four image variables stay in the assert on purpose. They are resolved
from the release vector by this role's own `setup_yaml` task, and role
argument validation is inserted BEFORE the first task, so marking them
`required` in the spec would fail on every run.

Split out of metal-stack#707 to keep the review small.
@GeertJohan
GeertJohan force-pushed the argument-specs/valkey branch from e8bd46b to 1c74477 Compare August 19, 2026 10:14
@Gerrit91

Copy link
Copy Markdown
Contributor

As we only take care of this specific role, I am fine to combine it with this PR. But we can also split it into two if it you want.

Comment on lines +89 to +91
valkey_resources:
type: dict
description: Resource requests and limits for the valkey container.

@vknabel vknabel Aug 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to add options here and / or in other places?
Of course we don't have to mirror k8s here and especially not all levels deep, but it might make sense in some cases to at least provide top-level attributes. The resources aren't necessarily a good example of this. Might be handy for upcoming PRs though :)

e.g.

options:
  resources:
    type: dict
  limits:
    type: dict
  claims:
    type: list
    elements: dict

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For certain variables this would be quite useful. As long as it does not prevent some degree of freedom, which might already be used by some consumers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: deployment Affects the deployment area.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants