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

feat: add WithDefault variants to groupBy template functions #607

Merged
merged 2 commits into from
May 5, 2024

Conversation

buchdag
Copy link
Member

@buchdag buchdag commented May 5, 2024

The PR add two new template function:

  • groupByWithDefault $containers $fieldPath $defaultValue
  • groupByLabelWithDefault $containers $label $defaultValue.

They both work as their counterpart groupBy and groupByLabel, but do not omit the containers that do not have a value for the provided field path expression or label, and instead group those under the provided $defaultValue.

I'm less convinced of the usefulness of a groupByMultiWithDefault template function, but maybe I'm wrong.

I'm not certain how a groupByKeysWithDefault $containers $fieldPath $defaultValue should work to be most useful and consistent:

  • return an array containing only the $defaultValue when no containers have the field path set ?
  • return an array containing the $defaultValue for every container that don't have the field path set ?

ping @pini-gh

@buchdag buchdag self-assigned this May 5, 2024
@pini-gh
Copy link
Contributor

pini-gh commented May 5, 2024

I'm less convinced of the usefulness of a groupByMultiWithDefault template function, but maybe I'm wrong.

It's not needed for nginx-proxy, but could be for other use cases.

I'm not certain how a groupByKeysWithDefault $containers $fieldPath $defaultValue should work to be most useful and consistent:

  • return an array containing the $defaultValue when at least one container doesn't have the field path set

@buchdag
Copy link
Member Author

buchdag commented May 5, 2024

This solution and the second one I proposed might cause issues in cases where we do stuff like this:

{{- $foot := or (first (groupByKeys $containers "Env.FOO")) "" }}

If we replace it with:

{{- $foot := first (groupByKeysWithDefault $containers "Env.FOO" "") }}

If the first container iterated over doesn't have the field path set, $foo is assigned the default value despite other containers having a value set for Env.FOO

Maybe we don't really need groupByKeysWithDefault right now because it does not allow us to substantially simplify the nginx-proxy template ?

@pini-gh
Copy link
Contributor

pini-gh commented May 5, 2024

If the first container iterated over doesn't have the field path set, $foo is assigned the default value despite other containers having a value set for Env.FOO

Yes, indeed. But it makes no sense to use groupByKeysWithDefault when the purpose is to get the first non-default value.

@buchdag
Copy link
Member Author

buchdag commented May 5, 2024

If that's okay with you I'll merge this PR as is and release a new version of docker-gen, then come back later for groupByMultiWithDefault and groupByKeysWithDefault.

In the context of nginx-proxy's template, I'm not really convinced groupByLabelWithDefault will immediately help us either because we're using groupByLabel kind of for the same purpose as groupByKeys (get the first set value of a given label or field path among a list of containers).

@pini-gh
Copy link
Contributor

pini-gh commented May 5, 2024

Yes that's fine.

@buchdag buchdag merged commit 8afa67b into main May 5, 2024
3 checks passed
@buchdag buchdag deleted the group-by-with-default branch May 5, 2024 13:51
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