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

Further options for filtering catalog services #532

Merged

Conversation

mmwinther
Copy link
Contributor

Functionality implemented

Supports filtering services based on Helm chart attributes:

  • keywords which must be present
  • keywords which cannot be present
  • annotations which must be present
  • annotations which cannot be present

Limitations

Currently can't include multiple values of the same annotation key (e.g. both lifecycle: production, lifecycle: experimental) since the values are supplied in a map and the keys would be duplicated. This could be improved in the future.

Commit summary

  • Move includeKeywords check to be a method on Chart
  • Filter services based on excluded keywords
  • Failing test for includeAnnotations
  • Implement filtering on includeAnnotations
  • Implement filtering on excludeAnnotations

Copy link
Contributor

@olevitt olevitt left a comment

Choose a reason for hiding this comment

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

Minor comment on the pattern used in your list getters. What do you think ?

}

public List<String> getExcludeKeywords() {
if (excludeKeywords == null) return new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a huge fan of this pattern of returning different references. In some cases it may leads to odd bugs if the caller then adds items to the returned list for example.
In one case the item would be added to the "real" list but if the list is null then the item would be added to a transient list and will not show up in a next get call.
Wouldn't it suffise to have default empty lists for each variable ? IIRC all objectmappers / json / yaml deserializers has been configured to set an empty list and not a null list so we should be safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olevitt Yes I agree, good point, I'll make that change 👍

@olevitt
Copy link
Contributor

olevitt commented Dec 17, 2024

Thank you ! 👍

@olevitt olevitt merged commit 165dbb3 into InseeFrLab:main Dec 17, 2024
6 checks passed
@johnksv johnksv deleted the feat/filter-catalog-services branch December 18, 2024 08:19
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.

2 participants