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.

Sign up for GitHub

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
merged 8 commits into from
Dec 17, 2024
Merged

Further options for filtering catalog services #532

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7c62c5e
Move includeKeywords check to be a method on Chart
mmwinther Nov 29, 2024
426badc
Filter services based on excluded keywords
mmwinther Nov 29, 2024
5a6a39a
Failing test for includeAnnotations
mmwinther Nov 29, 2024
2348f68
Implement filtering on includeAnnotations
mmwinther Nov 29, 2024
ace3ba6
Implement filtering on excludeAnnotations
mmwinther Dec 6, 2024
c4e24eb
Format with spotless
mmwinther Dec 6, 2024
3716afc
Format with spotless
mmwinther Dec 6, 2024
211ed7c
Remove null checks in getters
mmwinther Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions onyxia-api/src/main/java/fr/insee/onyxia/api/configuration/CatalogWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import fr.insee.onyxia.model.helm.Repository;
import io.swagger.v3.oas.annotations.media.Schema;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.regex.Pattern;

@JsonIgnoreProperties(ignoreUnknown = true)
Expand Down Expand Up @@ -54,6 +56,15 @@ public class CatalogWrapper {
@Schema(description = "only include charts with one or more of the given keywords")
private List<String> includeKeywords = new ArrayList<>();

@Schema(description = "exclude any charts which have one or more of the given keywords")
private List<String> excludeKeywords = new ArrayList<>();

@Schema(description = "only include charts with one or more of the given annotations")
private Map<String, String> includeAnnotations = new HashMap<>();

@Schema(description = "exclude any charts which have one or more of the given annotations")
private Map<String, String> excludeAnnotations = new HashMap<>();

@Schema(description = "Skip tls certificate checks for the repository")
private boolean skipTlsVerify;

Expand Down Expand Up @@ -198,13 +209,41 @@ public void setHighlightedCharts(List<String> highlightedCharts) {
}

public List<String> getIncludeKeywords() {
if (includeKeywords == null) return new ArrayList<>();
return includeKeywords;
}

public void setIncludeKeywords(List<String> includeKeywords) {
this.includeKeywords = includeKeywords;
}

public void setExcludeKeywords(List<String> excludeKeywords) {
this.excludeKeywords = excludeKeywords;
}

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 👍

return excludeKeywords;
}

public Map<String, String> getIncludeAnnotations() {
if (includeAnnotations == null) return new HashMap<>();
return includeAnnotations;
}

public void setIncludeAnnotations(Map<String, String> includeAnnotations) {
this.includeAnnotations = includeAnnotations;
}

public Map<String, String> getExcludeAnnotations() {
if (excludeAnnotations == null) return new HashMap<>();
return excludeAnnotations;
}

public void setExcludeAnnotations(Map<String, String> excludeAnnotations) {
this.excludeAnnotations = excludeAnnotations;
}

public boolean getSkipTlsVerify() {
return skipTlsVerify;
}
Expand Down
39 changes: 27 additions & 12 deletions onyxia-api/src/main/java/fr/insee/onyxia/api/dao/universe/CatalogLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.springframework.core.io.Resource;
import org.springframework.core.io.ResourceLoader;
import org.springframework.stereotype.Service;
import org.springframework.util.CollectionUtils;
import org.springframework.util.StringUtils;

@Service
Expand Down Expand Up @@ -76,19 +77,33 @@ private void updateHelmRepository(CatalogWrapper cw) {
excludedChart.equalsIgnoreCase(
entry.getKey())))
.filter(
// If includeKeywords is defined, include only services where
// the latest version has the desired keyword.
entry ->
cw.getIncludeKeywords() == null
|| cw.getIncludeKeywords().isEmpty()
|| cw.getIncludeKeywords().stream()
.anyMatch(
include ->
entry.getValue()
.getFirst()
.getKeywords()
.contains(
include)))
(CollectionUtils.isEmpty(cw.getIncludeKeywords())
|| entry.getValue()
.getFirst()
.hasKeywords(
cw
.getIncludeKeywords()))
&& (CollectionUtils.isEmpty(
cw.getIncludeAnnotations())
|| entry.getValue()
.getFirst()
.hasAnnotations(
cw
.getIncludeAnnotations())))
.filter(
entry ->
CollectionUtils.isEmpty(cw.getExcludeKeywords())
|| !entry.getValue()
.getFirst()
.hasKeywords(cw.getExcludeKeywords()))
.filter(
entry ->
CollectionUtils.isEmpty(cw.getExcludeAnnotations())
|| !entry.getValue()
.getFirst()
.hasAnnotations(
cw.getExcludeAnnotations()))
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)));
// For each service, filter the multiple versions if needed then refresh remaining
// versions
Expand Down
140 changes: 132 additions & 8 deletions onyxia-api/src/test/java/fr/insee/onyxia/api/dao/universe/CatalogLoaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.is;
import static org.junit.jupiter.api.Assertions.*;
import static org.junit.jupiter.params.provider.Arguments.arguments;
import static org.junit.jupiter.params.provider.Arguments.argumentSet;

import fr.insee.onyxia.api.configuration.CatalogWrapper;
import fr.insee.onyxia.api.configuration.CustomObjectMapper;
Expand All @@ -13,6 +13,7 @@
import fr.insee.onyxia.api.util.TestUtils;
import fr.insee.onyxia.model.helm.Chart;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand Down Expand Up @@ -161,21 +162,144 @@ void packageOnClassPathNotFound() {

@ParameterizedTest
@MethodSource("includeKeywords")
void filterIncludeKeywordsTest(List<String> includeKeywords, Set<String> expectedServices) {
@MethodSource("excludeKeywords")
@MethodSource("includeAnnotations")
@MethodSource("excludeAnnotations")
void filterServicesTest(
List<String> includeKeywords,
List<String> excludeKeywords,
Map<String, String> includeAnnotations,
Map<String, String> excludeAnnotations,
Set<String> expectedServices) {
CatalogWrapper cw = new CatalogWrapper();
cw.setType("helm");
cw.setLocation("classpath:/catalog-loader-test-with-keywords");
cw.setLocation("classpath:/catalog-loader-test-with-keywords-and-annotations");
cw.setIncludeKeywords(includeKeywords);
cw.setExcludeKeywords(excludeKeywords);
cw.setIncludeAnnotations(includeAnnotations);
cw.setExcludeAnnotations(excludeAnnotations);
catalogLoader.updateCatalog(cw);
assertEquals(expectedServices, cw.getCatalog().getEntries().keySet());
}

private static Stream<Arguments> includeKeywords() {
return Stream.of(
arguments(List.of("CD"), Set.of("keepme")),
arguments(List.of("CD", "Experimental"), Set.of("keepme", "excludeme")),
arguments(List.of(), Set.of("keepme", "excludeme")),
arguments(null, Set.of("keepme", "excludeme")),
arguments(List.of("no one knows"), Set.of()));
argumentSet(
"One keyword to include",
List.of("CD"),
null,
null,
null,
Set.of("keepme")),
argumentSet(
"Two keywords to include",
List.of("CD", "Experimental"),
null,
null,
null,
Set.of("keepme", "excludeme")),
argumentSet(
"Empty list of keywords to include",
List.of(),
null,
null,
null,
Set.of("keepme", "excludeme")),
argumentSet(
"null for all filters",
null,
null,
null,
null,
Set.of("keepme", "excludeme")),
argumentSet(
"Unknown keyword to include",
List.of("no one knows"),
null,
null,
null,
Set.of()));
}

private static Stream<Arguments> excludeKeywords() {
return Stream.of(
argumentSet(
"One keyword to exclude",
null,
List.of("Experimental"),
null,
null,
Set.of("keepme")),
argumentSet(
"Exclusive keywords to include and exclude",
List.of("CD"),
List.of("Experimental"),
null,
null,
Set.of("keepme")),
argumentSet(
"Keyword to exclude takes precedence",
List.of("Experimental"),
List.of("Experimental"),
null,
null,
Set.of()),
argumentSet(
"Two keywords to exclude",
List.of("CD"),
List.of("CD", "Experimental"),
null,
null,
Set.of()),
argumentSet(
"Empty lists of keywords to include and exclude",
List.of(),
List.of(),
null,
null,
Set.of("keepme", "excludeme")),
argumentSet(
"Unknown keyword to exclude",
null,
List.of("no one knows"),
null,
null,
Set.of("keepme", "excludeme")));
}

private static Stream<Arguments> includeAnnotations() {
return Stream.of(
argumentSet(
"One annotation to include",
null,
null,
Map.of("lifecycle", "production"),
null,
Set.of("keepme")),
argumentSet(
"Exclude keyword takes precedence",
null,
List.of("CD"),
Map.of("lifecycle", "production"),
null,
Set.of()));
}

private static Stream<Arguments> excludeAnnotations() {
return Stream.of(
argumentSet(
"One annotation to exclude",
null,
null,
null,
Map.of("lifecycle", "production"),
Set.of("excludeme")),
argumentSet(
"Exclude annotation takes precedence",
null,
null,
Map.of("lifecycle", "production"),
Map.of("lifecycle", "production"),
Set.of()));
}
}
0 ...-loader-test-with-keywords/catalogs.json5 → ...h-keywords-and-annotations/catalogs.json5
View file
Open in desktop
File renamed without changes.
8 changes: 8 additions & 0 deletions ...alog-loader-test-with-keywords/index.yaml → ...-with-keywords-and-annotations/index.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ entries:
maintainers:
- email: [email protected]
name: test
annotations:
lifecycle: production
- apiVersion: v2
appVersion: "1"
created: "2022-10-03T11:53:20.589754116Z"
Expand Down Expand Up @@ -55,6 +57,8 @@ entries:
maintainers:
- email: [email protected]
name: test
annotations:
lifecycle: production
- apiVersion: v2
appVersion: "2"
created: "2022-10-03T11:53:20.589754116Z"
Expand All @@ -79,6 +83,8 @@ entries:
urls:
- keepeme2.gz
version: 2.4.0
annotations:
lifecycle: experimental
excludeme:
- apiVersion: v2
appVersion: "1"
Expand Down Expand Up @@ -129,3 +135,5 @@ entries:
urls:
- excludeme2.gz
version: 2.4.0
annotations:
lifecycle: experimental
26 changes: 25 additions & 1 deletion onyxia-model/src/main/java/fr/insee/onyxia/model/helm/Chart.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
"name",
"sources",
"urls",
"version"
"version",
"annotations",
})
@Schema(description = "")
public class Chart extends Pkg {
Expand Down Expand Up @@ -277,6 +278,29 @@ public void setAdditionalProperty(String name, Object value) {
this.additionalProperties.put(name, value);
}

/**
* Does the chart have any of the given keywords?
*
* @param keywordsToCheck The list of keywords we're interested in.
* @return true if any of the given keywords appear in the keywords on the chart.
*/
public Boolean hasKeywords(List<String> keywordsToCheck) {
return getKeywords() != null
&& keywordsToCheck.stream().anyMatch(keyword -> getKeywords().contains(keyword));
}

/**
* Does the chart have any of the given annotations?
*
* @param annotationsToCheck The map of annotations we're interested in.
* @return true if any of the given annotations appear in the annotations on the chart.
*/
public Boolean hasAnnotations(Map<String, String> annotationsToCheck) {
return getAnnotations() != null
&& annotationsToCheck.entrySet().stream()
.anyMatch(annotation -> getAnnotations().entrySet().contains(annotation));
}

public static class Maintainer {
@JsonProperty("email")
private String email;
Expand Down
Loading