You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by brosander <gi...@git.apache.org> on 2016/10/18 20:27:59 UTC
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
GitHub user brosander opened a pull request:
https://github.com/apache/nifi-minifi/pull/45
MINIFI-117 - Maintainable Configuration Versioning
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/brosander/nifi-minifi MINIFI-117-3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi/pull/45.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #45
----
commit 1f8f6e605dce1ab5dfab10f7a267e46c7c727bfe
Author: Bryan Rosander <br...@gmail.com>
Date: 2016-10-07T17:23:21Z
MINIFI-117 - Maintainable Configuration Versioning
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85601260
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -336,6 +329,16 @@ public int transform(String[] args) {
return SUCCESS;
}
+ protected void validateAndPrintIssues(ConfigSchema configSchema) {
+ if (!configSchema.isValid()) {
+ System.out.println("There are validation errors with the template, still outputting YAML but it will need to be edited.");
+ configSchema.getValidationIssues().forEach(System.out::println);
--- End diff --
I don't think the validation issues are getting cleared properly along the way:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85128182
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -235,62 +252,127 @@ private static void addRemoteProcessGroupPortDTOs(Map<String, String> connectabl
}
}
+ public int upgrade(String[] args) {
+ if (args.length != 3) {
+ printUgradeUsage();
+ return ERR_INVALID_ARGS;
+ }
+
+ ConfigSchema configSchema = null;
+ try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
+ try {
+ configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
--- End diff --
After ingesting the config, it should check if it is already the latest version.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85604460
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -336,6 +329,16 @@ public int transform(String[] args) {
return SUCCESS;
}
+ protected void validateAndPrintIssues(ConfigSchema configSchema) {
+ if (!configSchema.isValid()) {
+ System.out.println("There are validation errors with the template, still outputting YAML but it will need to be edited.");
+ configSchema.getValidationIssues().forEach(System.out::println);
--- End diff --
Yup the first print is correct (was just including it to show what is wrong with the config). The print out for the upgrade could be more user friendly something like:
`
There were validation errors prior to upgrading:
<just the specified version errors>
After upgrading it has these validation errors:
<any errors after upgrading>
Any validation errors after upgrading will need to be addressed prior to using the config.
`
In its current state it looks like the upgraded template has validation errors for both destination ids and destination name (which should be mutually exclusive).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85601514
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -336,6 +329,16 @@ public int transform(String[] args) {
return SUCCESS;
}
+ protected void validateAndPrintIssues(ConfigSchema configSchema) {
+ if (!configSchema.isValid()) {
+ System.out.println("There are validation errors with the template, still outputting YAML but it will need to be edited.");
+ configSchema.getValidationIssues().forEach(System.out::println);
--- End diff --
@JPercivall the first printout looks right to me. You're missing the name in v1 which gets cleared. Then when it tries to create the v2 schema, it sees the id missing.
I can do similar logic with the upgrade command as well though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84529741
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/serialization/SchemaLoader.java ---
@@ -30,13 +32,13 @@
import java.util.stream.Collectors;
public class SchemaLoader {
- private static final Map<String, Function<Map, ConfigSchema>> configSchemaFactories = initConfigSchemaFactories();
+ private static final Map<String, Function<Map, ConvertableSchema<ConfigSchema>>> configSchemaFactories = initConfigSchemaFactories();
- private static Map<String, Function<Map, ConfigSchema>> initConfigSchemaFactories() {
- Map<String, Function<Map, ConfigSchema>> result = new HashMap<>();
- result.put(String.valueOf((Object)null), ConfigSchema::new);
- result.put("", ConfigSchema::new);
- result.put("1", ConfigSchema::new);
--- End diff --
Good point, I'll take a stab at updating the doc
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84527469
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/ConnectionSchema.java ---
@@ -60,30 +53,11 @@ public ConnectionSchema(Map map) {
super(map, CONNECTIONS_KEY);
sourceId = getOptionalKeyAsType(map, SOURCE_ID_KEY, String.class, CONNECTIONS_KEY, "");
--- End diff --
I believe this should now be required instead of optional (since there is no id vs name confusion)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85603814
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/serialization/SchemaLoader.java ---
@@ -78,6 +80,27 @@ public static ConfigSchema loadConfigSchemaFromYaml(Map<String, Object> yamlAsMa
public static ConvertableSchema<ConfigSchema> loadConvertableSchemaFromYaml(Map<String, Object> yamlAsMap) throws SchemaLoaderException {
String version = String.valueOf(yamlAsMap.get(ConfigSchema.VERSION));
+ if (StringUtil.isNullOrEmpty(version) || String.valueOf((Object) null).equals(version)) {
--- End diff --
Sorry my prior comment was confusing and I think it was a little off-base anyway. Lol feel free to let me know when I am (confusing or off-base).
That said, we need to be careful what we include in this SchemaLoader because it is not only used by the toolkit but also the agent itself. We don't want the agent assuming the version of schema.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84527193
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/ConfigSchema.java ---
@@ -142,128 +125,10 @@ public ConfigSchema(Map map) {
}
}
- protected List<ProcessorSchema> getProcessorSchemas(List<Map> processorMaps) {
- if (processorMaps == null) {
- return null;
- }
- List<ProcessorSchema> processors = convertListToType(processorMaps, "processor", ProcessorSchema.class, PROCESSORS_KEY);
-
- Map<String, Integer> idMap = processors.stream().map(ProcessorSchema::getId).filter(
- s -> !StringUtil.isNullOrEmpty(s)).collect(Collectors.toMap(Function.identity(), s -> 2, Integer::compareTo));
-
- // Set unset ids
- processors.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getId())).forEachOrdered(processor -> processor.setId(getUniqueId(idMap, processor.getName())));
-
- return processors;
- }
-
- protected List<ConnectionSchema> getConnectionSchemas(List<Map> connectionMaps) {
- if (connectionMaps == null) {
- return null;
- }
- List<ConnectionSchema> connections = convertListToType(connectionMaps, "connection", ConnectionSchema.class, CONNECTIONS_KEY);
- Map<String, Integer> idMap = connections.stream().map(ConnectionSchema::getId).filter(
- s -> !StringUtil.isNullOrEmpty(s)).collect(Collectors.toMap(Function.identity(), s -> 2, Integer::compareTo));
-
- Map<String, String> processorNameToIdMap = new HashMap<>();
-
- // We can't look up id by name for names that appear more than once
- Set<String> duplicateProcessorNames = new HashSet<>();
-
- List<ProcessorSchema> processors = getProcessors();
- if (processors != null) {
- processors.stream().forEachOrdered(p -> processorNameToIdMap.put(p.getName(), p.getId()));
-
- Set<String> processorNames = new HashSet<>();
- processors.stream().map(ProcessorSchema::getName).forEachOrdered(n -> {
- if (!processorNames.add(n)) {
- duplicateProcessorNames.add(n);
- }
- });
- }
-
- Set<String> remoteInputPortIds = new HashSet<>();
- List<RemoteProcessingGroupSchema> remoteProcessingGroups = getRemoteProcessingGroups();
- if (remoteProcessingGroups != null) {
- remoteInputPortIds.addAll(remoteProcessingGroups.stream().filter(r -> r.getInputPorts() != null)
- .flatMap(r -> r.getInputPorts().stream()).map(RemoteInputPortSchema::getId).collect(Collectors.toSet()));
- }
-
- Set<String> problematicDuplicateNames = new HashSet<>();
- Set<String> missingProcessorNames = new HashSet<>();
- // Set unset ids
- connections.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getId())).forEachOrdered(connection -> connection.setId(getUniqueId(idMap, connection.getName())));
-
- connections.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getSourceId())).forEach(connection -> {
- String sourceName = connection.getSourceName();
- if (remoteInputPortIds.contains(sourceName)) {
- connection.setSourceId(sourceName);
- } else {
- if (duplicateProcessorNames.contains(sourceName)) {
- problematicDuplicateNames.add(sourceName);
- }
- String sourceId = processorNameToIdMap.get(sourceName);
- if (StringUtil.isNullOrEmpty(sourceId)) {
- missingProcessorNames.add(sourceName);
- } else {
- connection.setSourceId(sourceId);
- }
- }
- });
-
- connections.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getDestinationId()))
- .forEach(connection -> {
- String destinationName = connection.getDestinationName();
- if (remoteInputPortIds.contains(destinationName)) {
- connection.setDestinationId(destinationName);
- } else {
- if (duplicateProcessorNames.contains(destinationName)) {
- problematicDuplicateNames.add(destinationName);
- }
- String destinationId = processorNameToIdMap.get(destinationName);
- if (StringUtil.isNullOrEmpty(destinationId)) {
- missingProcessorNames.add(destinationName);
- } else {
- connection.setDestinationId(destinationId);
- }
- }
- });
-
- if (problematicDuplicateNames.size() > 0) {
- addValidationIssue(CANNOT_LOOK_UP_PROCESSOR_ID_FROM_PROCESSOR_NAME_DUE_TO_DUPLICATE_PROCESSOR_NAMES
- + problematicDuplicateNames.stream().collect(Collectors.joining(", ")));
- }
- if (missingProcessorNames.size() > 0) {
- addValidationIssue(CONNECTIONS_REFER_TO_PROCESSOR_NAMES_THAT_DONT_EXIST + missingProcessorNames.stream().sorted().collect(Collectors.joining(", ")));
--- End diff --
I believe this and the above check were accidentally removed (this one should be "Id" instead of "name") when moving to the lambda functions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84488138
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -96,13 +99,21 @@ public int validate(String[] args) {
}
try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
try {
- ConfigSchema configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ ConvertableSchema<ConfigSchema> configSchema = SchemaLoader.loadConvertableSchemaFromYaml(inputStream);
if (!configSchema.isValid()) {
configSchema.getValidationIssues().forEach(s -> System.out.println(s));
System.out.println();
return ERR_INVALID_CONFIG;
} else {
- System.out.println(NO_VALIDATION_ERRORS_FOUND_IN_TEMPLATE);
+ ConfigSchema currentSchema = configSchema.convert();
--- End diff --
Would it make sense to convert and show any validation errors even if it's not valid in the old version?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84527495
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/ConnectionSchema.java ---
@@ -60,30 +53,11 @@ public ConnectionSchema(Map map) {
super(map, CONNECTIONS_KEY);
sourceId = getOptionalKeyAsType(map, SOURCE_ID_KEY, String.class, CONNECTIONS_KEY, "");
- if (StringUtil.isNullOrEmpty(sourceId)) {
- sourceName = getRequiredKeyAsType(map, SOURCE_NAME_KEY, String.class, CONNECTIONS_KEY);
- }
-
- String sourceRelationshipName = getOptionalKeyAsType(map, SOURCE_RELATIONSHIP_NAME_KEY, String.class, CONNECTIONS_KEY, null);
- if (StringUtil.isNullOrEmpty(sourceRelationshipName)) {
- sourceRelationshipNames = getOptionalKeyAsType(map, SOURCE_RELATIONSHIP_NAMES_KEY, List.class, CONNECTIONS_KEY, new ArrayList());
- if (sourceRelationshipNames.isEmpty()) {
- addValidationIssue(getIssueText(SOURCE_RELATIONSHIP_NAMES_KEY, CONNECTIONS_KEY, "expected at least one relationship to be specified"));
- }
- } else {
- if (map.containsKey(SOURCE_RELATIONSHIP_NAMES_KEY)) {
- addValidationIssue("Only one of " + SOURCE_RELATIONSHIP_NAME_KEY + ", " + SOURCE_RELATIONSHIP_NAMES_KEY + " should be set per connection. Found both on "
- + (StringUtil.isNullOrEmpty(getName()) ? getId() : getName()));
- sourceRelationshipNames = getRequiredKeyAsType(map, SOURCE_RELATIONSHIP_NAMES_KEY, List.class, CONNECTIONS_KEY);
- } else {
- sourceRelationshipNames = new ArrayList<>(Arrays.asList(sourceRelationshipName));
- }
+ sourceRelationshipNames = getOptionalKeyAsType(map, SOURCE_RELATIONSHIP_NAMES_KEY, List.class, CONNECTIONS_KEY, new ArrayList<>());
+ if (sourceRelationshipNames.isEmpty()) {
+ addValidationIssue("Expected at least one value in " + SOURCE_RELATIONSHIP_NAMES_KEY + " for " + CONNECTIONS_KEY + " " + getName());
}
-
destinationId = getOptionalKeyAsType(map, DESTINATION_ID_KEY, String.class, CONNECTIONS_KEY, "");
--- End diff --
Same comment as on source, I think this should be "required".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85137736
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -96,31 +102,39 @@ public int validate(String[] args) {
}
try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
try {
- ConfigSchema configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ ConvertableSchema<ConfigSchema> configSchema = SchemaLoader.loadConvertableSchemaFromYaml(inputStream);
+ boolean valid = true;
if (!configSchema.isValid()) {
+ System.out.println(FOUND_THE_FOLLOWING_ERRORS_WHEN_PARSING_THE_TEMPLATE_ACCORDING_TO_ITS_VERSION);
configSchema.getValidationIssues().forEach(s -> System.out.println(s));
System.out.println();
- return ERR_INVALID_CONFIG;
- } else {
+ valid = false;
+ configSchema.clearValidationIssues();
+ }
+
+ ConfigSchema currentSchema = configSchema.convert();
+ if (!currentSchema.isValid()) {
+ System.out.println(WHEN_CONVERTING_TO_LATEST_VERSION_FOUND_THE_FOLLOWING_ERRORS);
--- End diff --
This should state what the latest version for this toolkit is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85147924
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -96,31 +102,39 @@ public int validate(String[] args) {
}
try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
try {
- ConfigSchema configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ ConvertableSchema<ConfigSchema> configSchema = SchemaLoader.loadConvertableSchemaFromYaml(inputStream);
+ boolean valid = true;
if (!configSchema.isValid()) {
+ System.out.println(FOUND_THE_FOLLOWING_ERRORS_WHEN_PARSING_THE_TEMPLATE_ACCORDING_TO_ITS_VERSION);
--- End diff --
Good point, I'll add that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85137468
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -96,31 +102,39 @@ public int validate(String[] args) {
}
try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
try {
- ConfigSchema configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ ConvertableSchema<ConfigSchema> configSchema = SchemaLoader.loadConvertableSchemaFromYaml(inputStream);
+ boolean valid = true;
if (!configSchema.isValid()) {
+ System.out.println(FOUND_THE_FOLLOWING_ERRORS_WHEN_PARSING_THE_TEMPLATE_ACCORDING_TO_ITS_VERSION);
configSchema.getValidationIssues().forEach(s -> System.out.println(s));
System.out.println();
- return ERR_INVALID_CONFIG;
- } else {
+ valid = false;
+ configSchema.clearValidationIssues();
+ }
+
+ ConfigSchema currentSchema = configSchema.convert();
--- End diff --
Something that's a bit confusing to the user, I had a valid v2 config file and commented out the version line. When I did this I properly saw that there were validation errors when validating it for it's current version (v1) but didn't expect to see errors when "converting" it to the current version. I completely understand why (the connection ids were not ingested) but maybe either the second check needs to change or a third where it explicitly tries to load it with the latest schema.
Or I could be completely off base, lol, let me know what you think.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84531003
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/ConfigSchema.java ---
@@ -142,128 +125,10 @@ public ConfigSchema(Map map) {
}
}
- protected List<ProcessorSchema> getProcessorSchemas(List<Map> processorMaps) {
- if (processorMaps == null) {
- return null;
- }
- List<ProcessorSchema> processors = convertListToType(processorMaps, "processor", ProcessorSchema.class, PROCESSORS_KEY);
-
- Map<String, Integer> idMap = processors.stream().map(ProcessorSchema::getId).filter(
- s -> !StringUtil.isNullOrEmpty(s)).collect(Collectors.toMap(Function.identity(), s -> 2, Integer::compareTo));
-
- // Set unset ids
- processors.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getId())).forEachOrdered(processor -> processor.setId(getUniqueId(idMap, processor.getName())));
-
- return processors;
- }
-
- protected List<ConnectionSchema> getConnectionSchemas(List<Map> connectionMaps) {
- if (connectionMaps == null) {
- return null;
- }
- List<ConnectionSchema> connections = convertListToType(connectionMaps, "connection", ConnectionSchema.class, CONNECTIONS_KEY);
- Map<String, Integer> idMap = connections.stream().map(ConnectionSchema::getId).filter(
- s -> !StringUtil.isNullOrEmpty(s)).collect(Collectors.toMap(Function.identity(), s -> 2, Integer::compareTo));
-
- Map<String, String> processorNameToIdMap = new HashMap<>();
-
- // We can't look up id by name for names that appear more than once
- Set<String> duplicateProcessorNames = new HashSet<>();
-
- List<ProcessorSchema> processors = getProcessors();
- if (processors != null) {
- processors.stream().forEachOrdered(p -> processorNameToIdMap.put(p.getName(), p.getId()));
-
- Set<String> processorNames = new HashSet<>();
- processors.stream().map(ProcessorSchema::getName).forEachOrdered(n -> {
- if (!processorNames.add(n)) {
- duplicateProcessorNames.add(n);
- }
- });
- }
-
- Set<String> remoteInputPortIds = new HashSet<>();
- List<RemoteProcessingGroupSchema> remoteProcessingGroups = getRemoteProcessingGroups();
- if (remoteProcessingGroups != null) {
- remoteInputPortIds.addAll(remoteProcessingGroups.stream().filter(r -> r.getInputPorts() != null)
- .flatMap(r -> r.getInputPorts().stream()).map(RemoteInputPortSchema::getId).collect(Collectors.toSet()));
- }
-
- Set<String> problematicDuplicateNames = new HashSet<>();
- Set<String> missingProcessorNames = new HashSet<>();
- // Set unset ids
- connections.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getId())).forEachOrdered(connection -> connection.setId(getUniqueId(idMap, connection.getName())));
-
- connections.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getSourceId())).forEach(connection -> {
- String sourceName = connection.getSourceName();
- if (remoteInputPortIds.contains(sourceName)) {
- connection.setSourceId(sourceName);
- } else {
- if (duplicateProcessorNames.contains(sourceName)) {
- problematicDuplicateNames.add(sourceName);
- }
- String sourceId = processorNameToIdMap.get(sourceName);
- if (StringUtil.isNullOrEmpty(sourceId)) {
- missingProcessorNames.add(sourceName);
- } else {
- connection.setSourceId(sourceId);
- }
- }
- });
-
- connections.stream().filter(connection -> StringUtil.isNullOrEmpty(connection.getDestinationId()))
- .forEach(connection -> {
- String destinationName = connection.getDestinationName();
- if (remoteInputPortIds.contains(destinationName)) {
- connection.setDestinationId(destinationName);
- } else {
- if (duplicateProcessorNames.contains(destinationName)) {
- problematicDuplicateNames.add(destinationName);
- }
- String destinationId = processorNameToIdMap.get(destinationName);
- if (StringUtil.isNullOrEmpty(destinationId)) {
- missingProcessorNames.add(destinationName);
- } else {
- connection.setDestinationId(destinationId);
- }
- }
- });
-
- if (problematicDuplicateNames.size() > 0) {
- addValidationIssue(CANNOT_LOOK_UP_PROCESSOR_ID_FROM_PROCESSOR_NAME_DUE_TO_DUPLICATE_PROCESSOR_NAMES
- + problematicDuplicateNames.stream().collect(Collectors.joining(", ")));
- }
- if (missingProcessorNames.size() > 0) {
- addValidationIssue(CONNECTIONS_REFER_TO_PROCESSOR_NAMES_THAT_DONT_EXIST + missingProcessorNames.stream().sorted().collect(Collectors.joining(", ")));
--- End diff --
I agree we should validate that the connections refer to existing ids, will add that
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85127907
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -235,62 +252,127 @@ private static void addRemoteProcessGroupPortDTOs(Map<String, String> connectabl
}
}
+ public int upgrade(String[] args) {
+ if (args.length != 3) {
+ printUgradeUsage();
+ return ERR_INVALID_ARGS;
+ }
+
+ ConfigSchema configSchema = null;
+ try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
+ try {
+ configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ } catch (IOException|SchemaLoaderException e) {
+ return handleErrorLoadingConfiguration(e, ConfigMain::printUgradeUsage);
+ }
+ } catch (FileNotFoundException e) {
+ return handleErrorOpeningInput(args[1], ConfigMain::printUgradeUsage, e);
+ } catch (IOException e) {
+ handleErrorClosingInput(e);
+ }
+
+ try (OutputStream fileOutputStream = pathOutputStreamFactory.create(args[2])) {
+ try {
+ SchemaSaver.saveConfigSchema(configSchema, fileOutputStream);
+ } catch (IOException e) {
+ return handleErrorSavingCofiguration(e);
+ }
+ } catch (FileNotFoundException e) {
+ return handleErrorOpeningOutput(args[2], ConfigMain::printUgradeUsage, e);
+ } catch (IOException e) {
+ handleErrorClosingOutput(e);
+ }
+
+ return SUCCESS;
--- End diff --
This should run a validate on the resulting upgraded file. I ran a test where I upgraded a flow with a connection that was missing a destination and I was confused when it didn't tell there were problems.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85601256
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/serialization/SchemaLoader.java ---
@@ -78,6 +80,27 @@ public static ConfigSchema loadConfigSchemaFromYaml(Map<String, Object> yamlAsMa
public static ConvertableSchema<ConfigSchema> loadConvertableSchemaFromYaml(Map<String, Object> yamlAsMap) throws SchemaLoaderException {
String version = String.valueOf(yamlAsMap.get(ConfigSchema.VERSION));
+ if (StringUtil.isNullOrEmpty(version) || String.valueOf((Object) null).equals(version)) {
--- End diff --
@JPercivall sorry, I thought you were wanting behavior like that in regards to [your comment above.](https://github.com/apache/nifi-minifi/pull/45#discussion_r85137468)
I must've misread the comment, now it's looking more like it may have been automatically addressed by the other validation change which clears the errors before converting.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85600674
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/serialization/SchemaLoader.java ---
@@ -78,6 +80,27 @@ public static ConfigSchema loadConfigSchemaFromYaml(Map<String, Object> yamlAsMa
public static ConvertableSchema<ConfigSchema> loadConvertableSchemaFromYaml(Map<String, Object> yamlAsMap) throws SchemaLoaderException {
String version = String.valueOf(yamlAsMap.get(ConfigSchema.VERSION));
+ if (StringUtil.isNullOrEmpty(version) || String.valueOf((Object) null).equals(version)) {
--- End diff --
@brosander what was the motivation to move all of this logging here and potentially change which yaml version gets used? I preferred keeping the version as a required property and assuming that null and "" were v1 (makes things much simpler in the long run).
A side effect of the logging is weird messages when doing simple validation or conversion using the toolkit (test.yml has no config version listed):
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85142121
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/serialization/SchemaLoader.java ---
@@ -30,13 +32,13 @@
import java.util.stream.Collectors;
public class SchemaLoader {
--- End diff --
When you attempt to load a schema version that's not listed you see this error message (gotten via config.sh validate):
YAML configuration version 3 not supported. Supported versions: , 1, 2, null)
"" and "Null" are more implementation details that the user doesn't need to know about. Somewhere we should say that a missing version number = v1.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85150769
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/serialization/SchemaLoader.java ---
@@ -30,13 +32,13 @@
import java.util.stream.Collectors;
public class SchemaLoader {
--- End diff --
@JPercivall it would be pretty straightforward to filter null or empty values from that printout which is the only place that map is enumerated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85137652
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -96,31 +102,39 @@ public int validate(String[] args) {
}
try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
try {
- ConfigSchema configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ ConvertableSchema<ConfigSchema> configSchema = SchemaLoader.loadConvertableSchemaFromYaml(inputStream);
+ boolean valid = true;
if (!configSchema.isValid()) {
+ System.out.println(FOUND_THE_FOLLOWING_ERRORS_WHEN_PARSING_THE_TEMPLATE_ACCORDING_TO_ITS_VERSION);
--- End diff --
This should state what the version of config file is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84529455
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -96,13 +99,21 @@ public int validate(String[] args) {
}
try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
try {
- ConfigSchema configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ ConvertableSchema<ConfigSchema> configSchema = SchemaLoader.loadConvertableSchemaFromYaml(inputStream);
if (!configSchema.isValid()) {
configSchema.getValidationIssues().forEach(s -> System.out.println(s));
System.out.println();
return ERR_INVALID_CONFIG;
} else {
- System.out.println(NO_VALIDATION_ERRORS_FOUND_IN_TEMPLATE);
+ ConfigSchema currentSchema = configSchema.convert();
--- End diff --
I think it would, good call :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84530471
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/ConnectionSchema.java ---
@@ -60,30 +53,11 @@ public ConnectionSchema(Map map) {
super(map, CONNECTIONS_KEY);
sourceId = getOptionalKeyAsType(map, SOURCE_ID_KEY, String.class, CONNECTIONS_KEY, "");
--- End diff --
Ah I see, could you just add a comment above these two "optional" calls explaining that?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi-minifi/pull/45
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by JPercivall <gi...@git.apache.org>.
Github user JPercivall commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84528235
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/serialization/SchemaLoader.java ---
@@ -30,13 +32,13 @@
import java.util.stream.Collectors;
public class SchemaLoader {
- private static final Map<String, Function<Map, ConfigSchema>> configSchemaFactories = initConfigSchemaFactories();
+ private static final Map<String, Function<Map, ConvertableSchema<ConfigSchema>>> configSchemaFactories = initConfigSchemaFactories();
- private static Map<String, Function<Map, ConfigSchema>> initConfigSchemaFactories() {
- Map<String, Function<Map, ConfigSchema>> result = new HashMap<>();
- result.put(String.valueOf((Object)null), ConfigSchema::new);
- result.put("", ConfigSchema::new);
- result.put("1", ConfigSchema::new);
--- End diff --
The changes in the Config schemas need to be documented. Could you add a section to the admin guide (the Config Schema portion) that explains the versioning scheme as well as what changed in v2?
I am suggesting to put it in the docs instead of a migration guidance for a release because working with multiple different version of MiNiFi and configs will be an everyday task (esp when C&C is added).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r84529673
--- Diff: minifi-commons/minifi-commons-schema/src/main/java/org/apache/nifi/minifi/commons/schema/ConnectionSchema.java ---
@@ -60,30 +53,11 @@ public ConnectionSchema(Map map) {
super(map, CONNECTIONS_KEY);
sourceId = getOptionalKeyAsType(map, SOURCE_ID_KEY, String.class, CONNECTIONS_KEY, "");
--- End diff --
It is treated as required in the getValidationIssues implementation. I needed a chance to set it when upconverting before the validation occurs. I could change that to map manipulation before instantiating the ConnectionSchema but that felt worse.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi-minifi pull request #45: MINIFI-117 - Maintainable Configuration Versio...
Posted by brosander <gi...@git.apache.org>.
Github user brosander commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/45#discussion_r85150335
--- Diff: minifi-toolkit/minifi-toolkit-configuration/src/main/java/org/apache/nifi/minifi/toolkit/configuration/ConfigMain.java ---
@@ -235,62 +252,127 @@ private static void addRemoteProcessGroupPortDTOs(Map<String, String> connectabl
}
}
+ public int upgrade(String[] args) {
+ if (args.length != 3) {
+ printUgradeUsage();
+ return ERR_INVALID_ARGS;
+ }
+
+ ConfigSchema configSchema = null;
+ try (InputStream inputStream = pathInputStreamFactory.create(args[1])) {
+ try {
+ configSchema = SchemaLoader.loadConfigSchemaFromYaml(inputStream);
+ } catch (IOException|SchemaLoaderException e) {
+ return handleErrorLoadingConfiguration(e, ConfigMain::printUgradeUsage);
+ }
+ } catch (FileNotFoundException e) {
+ return handleErrorOpeningInput(args[1], ConfigMain::printUgradeUsage, e);
+ } catch (IOException e) {
+ handleErrorClosingInput(e);
+ }
+
+ try (OutputStream fileOutputStream = pathOutputStreamFactory.create(args[2])) {
+ try {
+ SchemaSaver.saveConfigSchema(configSchema, fileOutputStream);
+ } catch (IOException e) {
+ return handleErrorSavingCofiguration(e);
+ }
+ } catch (FileNotFoundException e) {
+ return handleErrorOpeningOutput(args[2], ConfigMain::printUgradeUsage, e);
+ } catch (IOException e) {
+ handleErrorClosingOutput(e);
+ }
+
+ return SUCCESS;
--- End diff --
Good point, adding
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---