apache / hop

Hop Orchestration Platform
https://hop.apache.org/
Apache License 2.0
983 stars 352 forks source link

[Bug]: 'Open File' dialog when editing configuration files in Environment properties prepends CWD of client, doesn't resolve variables #3324

Open kranskydog opened 1 year ago

kranskydog commented 1 year ago

Apache Hop version?

2.6.0

Java version?

openjdk version "17.0.8" 2023-07-18 LTS OpenJDK Runtime Environment (Red_Hat-17.0.8.0.7-2.0.1) (build 17.0.8+7-LTS) OpenJDK 64-Bit Server VM (Red_Hat-17.0.8.0.7-2.0.1) (build 17.0.8+7-LTS, mixed mode, sharing)

Operating system

Linux

What happened?

This occurs in both Hop-GUI and Hop-Web clients

1) Edit an Environment 2) click 'New' to create a new file, or, if you have existing files which have and environment variable in the path (ie ${HOP_CONFIG_FOLDER}) choose that file and click 'Select 3) the 'Open File' dialog will open with the path of the CWD prepended to the path of the selected configuration file, and/or without any environment variables being replaced

image image image

Issue Priority

Priority: 3

Issue Component

Component: Hop Gui, Component: Hop Web

mattcasters commented 1 year ago

So I guess this is a feature request to allow variables to be replaced in the Open File dialog? Should we retain the value somehow or just replace the expression with its value?

kranskydog commented 1 year ago

It is trying to look for a file. How is it going to find the file if it is looking under the literal variable name, instead of the value of the variable?

similarly, when it does substitute the value of the variable, when that value is an absolute path, prepending the CWD of the client isn't going to help

sramazzina commented 12 months ago

@hansva looking at the last reference I think we can close this issue, isn't it?

kranskydog commented 12 months ago

@sramazzina @hansva That reference is wrong @mattcasters there is a typo in the description of your pull request #3324 -> #3234

hansva commented 12 months ago

yup, this can stay open.

hansva commented 12 months ago

Actually, I just tried this and variable replacement works. When defining a variable in hop-config.json it can be used in the environments dialog. Where did you specify your HOP_CONFIG_FOLDER?

kranskydog commented 12 months ago

As far as I am aware, there is only one place it needs to be set - the user's environment before they launch the GUI, or the environment of the application server for Hop Web https://hop.apache.org/manual/latest/installation-configuration.html#_the_environment_variables_to_set

hansva commented 12 months ago

That expansion also works on my system and it uses the correct variable value...

hans@MacBook-Pro-2:~|⇒  echo $HOP_CONFIG_FOLDER
/Users/hans/config

image

kranskydog commented 12 months ago

@hansva and what happens when you hit 'Select' on that file?

hansva commented 12 months ago

No issue there for me. Do I see it correctly that your hop_config_directory points to a smb/windows share //u01 ? Because that might be the cause of all issues, we have known issues with UNC paths.

kranskydog commented 12 months ago

@hansva No, it's all on Linux

kranskydog commented 12 months ago

somehow it seems to be remembering where it was last time, because when I hit 'Select' I got image which the last path I used to save a pipeline But then, while I am here image if I hit 'New', I get image and then if I hit 'Select' again, I get image

So, simply doing it once isn't necessarily going to reproduce the problem

kranskydog commented 12 months ago

for reference [apache@apchop01 ~]$ pwd /home/apache [apache@apchop01 ~]$ echo $HOP_CONFIG_FOLDER /u01/app/hop/admin/config [apache@apchop01 ~]$ /u01/app/hop/product/hop-client-2.6.0/hop-gui.sh

kranskydog commented 12 months ago

Note, also "projectName" : "oracle", "projectHome" : "${HOP_CONFIG_FOLDER}/projects/oracle", "configFilename" : "project-config.json" } ], "lifecycleEnvironments" : [ { "name" : "XE", "purpose" : "Development", "projectName" : "oracle", "configurationFiles" : [ "${PROJECT_HOME}/environments/XE-config.json", "${PROJECT_HOME}/environments/XE-passwd.json" ] }, { "name" : "ORCL", "purpose" : "Production", "projectName" : "oracle", "configurationFiles" : [ "${PROJECT_HOME}/environments/ORCL-config.json", "${PROJECT_HOME}/environments/COMMON-config.json" ]

hansva commented 12 months ago

Aha, I was able to reproduce now.

I was missing 1 small part of your puzzle. which seems to be the root cause of all your problems.

Project home does not support variables. Or at least on expansion, it is unable to replace the nested parameter ${PROJECT_HOME} expands to ${HOP_CONFIG_FOLDER}/projects/oracle and then the ${HOP_CONFIG_FOLDER} is not replaced by the value.

When pressing the new button it tries to go to ${PROJECT_HOME} it can't find the file location and we have a fallback in place to go to PWD if locations are not found and this ends up in your weird path.

In the file dialog when pressing the "P" button in the top left button it should go to your project root, but this also fails.

As for your comment on Select, yes the dialog remembers the last known location and goes there by default. The starting point used to be project root always. but we had complaints from users that were working in a folder that they always had to browse there. So now we remember the last location and use that as starting point. We have buttons in the top left to go to user home or project root.

kranskydog commented 11 months ago

"Project home does not support variables."

so, if I move my HOP_CONFIG_FOLDER, which includes all my project homes, instead of just changing the value of HOP_CONFIG_FOLDER, and having it Just Work(TM), I also have to go through and change all the values of PROJECT_HOME, because it isn't smart enough to resolve the HOP_CONFIG_FOLDER variable.

mattcasters commented 11 months ago

It's just a strange design you made, one we didn't encounter yet. That's all really, Phil. Most people would keep configuration and project metadata separate in the light of putting both in separate version control repositories. We didn't say we wouldn't fix it.

kranskydog commented 11 months ago

Ahem...

[apache@apchop01 config]$ pwd /u01/app/hop/product/hop-client-2.6.0/config [apache@apchop01 config]$ ls hop-config.json projects [apache@apchop01 config]$ ls projects default samples

[apache@apchop01 config]$ pwd /u01/app/hop/admin/config [apache@apchop01 config]$ ls hop-config.json metadata projects [apache@apchop01 config]$ ls projects/ default oracle samples

I am literally following the example provided in the standard distribution.

Not to mention (from https://hop.apache.org/dev-manual/latest/hopweb/index.html#_modify_startup_script) image

mattcasters commented 11 months ago

Just to clarify further Phil, we absolutely do not recommend that you use the default configuration. But, I guess it's great that we now know how you got to your setup.

kranskydog commented 11 months ago

so what you're telling me is HOP_CONFIG_FOLDER should only contain one file.

mattcasters commented 11 months ago

Thanks for helping us with the reproduction of the issue Phil. It's been informative.

kranskydog commented 11 months ago

Cf image

hansva commented 11 months ago

It's valid usage and it should work, so we will fix it.

kranskydog commented 11 months ago

Just for some background, I am not a developer, I am an infrastructure person (I manage a large Oracle database installation). So my POV comes from what is the architecture, how would I deploy this, and what are the infrastructure lifecycle management implications. I am not so concerned with things like managing the development lifecycle - that's S.E.P. One big thing I try to use is 'single source of truth', and the corollary that things should only have one place where they are defined, and then that definition can be used by reference. If it changes in the architecture, it should only have to be changed once in the configuration, and everything else that references it Just Works(TM)

So I am not trying to be unnecessarily antagonistic, I am just passionate about well designed and managed infrastructure. I am hopeful that one day HOP may have a place in that where I work.

The other thing is that while I am technically adept at many things, I come to this as a novice, so will take what is in the documentation and the provided examples at face value.

mattcasters commented 11 months ago

Thanks again for the background information Phil. As a project Apache Hop doesn't really want to dictate a preferred way to set up, use or install. At that same time, the people that built it, decided on some things in the past. Making Apache Hop doing a nested variable resolution here will surely help more than just you and open up more possibilities. I would personally limit the solution to this particular HOP_CONFIG_FOLDER resolution. I'll create another issue to see how we can make nested variable resolution more generic and configurable across the platform.

mattcasters commented 11 months ago

Created issue #3454