mattcasters commented on code in PR #8587:
URL: https://github.com/apache/hop/pull/8587#discussion_r4106450479
##########
engine/src/main/java/org/apache/hop/imp/HopImport.java:
##########
@@ -260,6 +292,75 @@ private IHopImport loadImportPlugin() throws HopException {
return hi;
}
+ /**
+ * A missing run configuration name no longer blanks the one the source file
carried, but the
+ * source may not have carried one either. Say so, the way the import dialog
does.
+ */
+ private void warnAboutMissingRunConfiguration(String subject, String
runConfigurationName) {
+ if (StringUtils.isEmpty(runConfigurationName)) {
+ log.logBasic(
+ "No default "
+ + subject
+ + " run configuration was specified. Imported "
+ + subject
+ + "s keep the run configuration named in the source file, which
can be empty.");
+ }
+ }
+
+ /**
+ * Point the import at a project: an existing one contributes its home
folder as the target, an
+ * unknown one is registered at the target folder. Both go through the
extension points the import
+ * dialog uses, so this is a no-op without the projects plugin.
+ */
+ private void resolveTargetProject() throws HopException {
+ if (StringUtils.isEmpty(projectName)) {
+ return;
+ }
+ String projectHome = findProjectHome(projectName);
+ if (StringUtils.isNotEmpty(projectHome)) {
+ if (StringUtils.isNotEmpty(outputFolderName) &&
!projectHome.equals(outputFolderName)) {
+ log.logBasic(
+ "Ignoring output folder '"
+ + outputFolderName
+ + "': project '"
+ + projectName
+ + "' is imported into its own home folder");
+ }
+ outputFolderName = projectHome;
+ log.logBasic("Importing into project '" + projectName + "' at " +
projectHome);
+ return;
+ }
+ if (StringUtils.isEmpty(outputFolderName)) {
+ // validateOptions() reports the missing output folder.
+ return;
+ }
+ ExtensionPointHandler.callExtensionPoint(
+ log,
+ variables,
+ HopExtensionPoint.HopImportCreateProject.id,
+ new Object[] {outputFolderName, projectName});
Review Comment:
**[suggestion]** The folder stored as the new project's home is the raw `-o`
string. It is not variable-resolved and not made absolute. `ProjectHome` later
returns that string (only `${...}` substituted), and the next `hop-import
--project` without `-o` uses it as the output folder. A relative `-o out` is
then resolved against the new process's working directory, not the directory of
the first import.
**Suggestion:** Resolve variables, then store the absolute path from
`HopVfs.getFileObject(...).getName().getPath()` before calling the extension
point, so the saved home does not depend on the later working directory.
##########
engine/src/main/java/org/apache/hop/imp/HopImport.java:
##########
@@ -260,6 +292,75 @@ private IHopImport loadImportPlugin() throws HopException {
return hi;
}
+ /**
+ * A missing run configuration name no longer blanks the one the source file
carried, but the
+ * source may not have carried one either. Say so, the way the import dialog
does.
+ */
+ private void warnAboutMissingRunConfiguration(String subject, String
runConfigurationName) {
+ if (StringUtils.isEmpty(runConfigurationName)) {
+ log.logBasic(
+ "No default "
+ + subject
+ + " run configuration was specified. Imported "
+ + subject
+ + "s keep the run configuration named in the source file, which
can be empty.");
+ }
+ }
+
+ /**
+ * Point the import at a project: an existing one contributes its home
folder as the target, an
+ * unknown one is registered at the target folder. Both go through the
extension points the import
+ * dialog uses, so this is a no-op without the projects plugin.
+ */
+ private void resolveTargetProject() throws HopException {
+ if (StringUtils.isEmpty(projectName)) {
+ return;
+ }
+ String projectHome = findProjectHome(projectName);
+ if (StringUtils.isNotEmpty(projectHome)) {
+ if (StringUtils.isNotEmpty(outputFolderName) &&
!projectHome.equals(outputFolderName)) {
+ log.logBasic(
+ "Ignoring output folder '"
+ + outputFolderName
+ + "': project '"
+ + projectName
+ + "' is imported into its own home folder");
+ }
+ outputFolderName = projectHome;
+ log.logBasic("Importing into project '" + projectName + "' at " +
projectHome);
+ return;
+ }
+ if (StringUtils.isEmpty(outputFolderName)) {
+ // validateOptions() reports the missing output folder.
+ return;
+ }
+ ExtensionPointHandler.callExtensionPoint(
+ log,
+ variables,
+ HopExtensionPoint.HopImportCreateProject.id,
+ new Object[] {outputFolderName, projectName});
+ if (StringUtils.isEmpty(findProjectHome(projectName))) {
+ log.logError(
Review Comment:
**[suggestion]** If `--project` was requested and the project still cannot
be resolved, this logs an error and continues. `run()` then sets
`finishedWithoutError`, so `main()` exits 0. A caller that only checks the exit
code treats a failed registration as success.
**Suggestion:** When `projectName` is set and this follow-up
`findProjectHome` is still empty, leave `finishedWithoutError` false or throw,
so the process exits non-zero. The log line can stay.
##########
plugins/misc/import/src/main/java/org/apache/hop/imports/kettle/KettleImport.java:
##########
@@ -1008,9 +1013,11 @@ else if (entryType == EntryType.TRANS)
}
// add the default pipeline run configuration.
- Element runConfigElement = doc.createElement("runConfiguration");
-
runConfigElement.appendChild(doc.createTextNode(defaultPipelineRunConfiguration));
- currentNode.appendChild(runConfigElement);
+ if (StringUtils.isNotEmpty(defaultPipelineRunConfiguration)) {
+ Element runConfigElement = doc.createElement("runConfiguration");
Review Comment:
**[suggestion]** When a default is set, this still always appends an element
named `runConfiguration`, for both `SIMPLE_MAPPING` and `METAINJECT`, and it
does not look for an existing child. `SimpleMappingMeta` and `MultiMappingMeta`
use `runConfiguration`, but `MetaInjectMeta` uses `run_configuration`.
`--pipeline-run-configuration` (and the same dialog field) therefore never sets
the run configuration Hop loads for a Metadata Injection transform, and a
mapping that already has the element gets a second one.
**Suggestion:** Choose `run_configuration` for `EntryType.METAINJECT` and
`runConfiguration` otherwise. If that child already exists, replace its text
when a default is set; otherwise add it only when `getChildElement` returns
null.
##########
engine/src/main/java/org/apache/hop/imp/HopImport.java:
##########
@@ -166,9 +187,12 @@ public void run() {
if (listPluginTypes != null && listPluginTypes) {
printPluginTypes();
+ finishedWithoutError = true;
return;
}
+ resolveTargetProject();
Review Comment:
**[bug]** `resolveTargetProject()` persists a new project before the import
is known to be viable. It runs before `validateOptions()`, and also before
`setValidateInputFolder()` / `loadImportPlugin()`. `HopImportCreateProject`
saves `hop-config.json` and writes `project-config.json` immediately.
`hop-import --project New -o /tmp/out` with no `-i`, an `-i` that does not
exist, or a `--type` that does not load, still registers `New` and then exits
1. A later `hop-import --project New` (no `-o`) imports into that folder.
**Suggestion:** Check the required options and resolve both folders
successfully first. Call the create-project extension point only after
`setValidateInputFolder` and `setValidateOutputFolder` have succeeded, and do
not leave the project registered if setup then fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
