Add target_path flag option for edr files and logs by noaKurman · Pull Request #747 · elementary-data/elementary

noaKurman · 2023-03-13T15:32:32Z

No description provided.

linear · 2023-03-13T15:32:35Z

ELE-407 edr should have a target folder for all outputs

"It seems quite messy for to have the 3 files created for the report in the main project folder e.g.

edr.log
elementary_output.json
elementary_report.html

In most other packages where documentation has been built, all the artifacts created from the build have been added to a separate sub-folder (e.g. docs or build or dist or something along those lines). What I would like to be able to do is define within my project folder where the three edr files are built, so that I can ensure that is not included in source control, and to ensure that folder is consistent across all developers.
As it is, the behaviour of target-path in the config file doesn't quite do what I want it to, as the log is still written to the main project folder."

Slack thread:
https://elementary-community.slack.com/archives/C02CTC89LAX/p1674490912619819

github-actions · 2023-03-13T15:32:45Z

👋 @noaKurman
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

RoiTabach · 2023-03-13T16:30:18Z

Looks good, added one small comment
Do we want to add user-facing documentation for it?

Maayan-s · 2023-03-15T06:32:43Z

Looks good, added one small comment
Do we want to add user-facing documentation for it?

@noaKurman
If you don't document no one will use it, so it's not a question 🙃
Let me know if you need help with docs.
You can do it in different PR

noaKurman · 2023-03-15T07:41:38Z

Looks good, added one small comment
Do we want to add user-facing documentation for it?

@noaKurman If you don't document no one will use it, so it's not a question 🙃 Let me know if you need help with docs. You can do it in different PR

@Maayan-s @RoiTabach
I added documentation in docs/understand-elementary/cli-commands.mdx, is that enough? If not I'll add it in a different PR :)

Maayan-s · 2023-03-15T07:47:56Z

+```
+
+- **Configuration target directory path**
+  The path where all output files will be saved, default will create "edr_files" directory in your path


@noaKurman
Can they change the default?
If not I would change to:
The path where edr should create an edr_files directory where all output files will be saved.

Not completely sure what you meant:
You can either run --target-path ./noa-target and it would create noa-target in the current directory and save all files there, or you can ignore the flag and it would create edr_files in your current directory and save all files there, so should I change it to the phrase you suggested?
Also should I mention it's an absolute path in the doc?
@Maayan-s

Ok I get it now! @noaKurman

So I would write like this:
The absolute path where all output files such as logs and reports will be saved. If not configured, the default is ./edr_files.

RoiTabach

Looks good. Found 2 small typos, amazingly both don't have bad side effects but still best to remove them. Also wondering if we should rename Customer->Custom .

Afterwards - Yalla merge (Also write something in slack #tech so the team knows since it's a behavioural change)

RoiTabach · 2023-03-15T12:11:17Z

        config = self._load_configuration()

-        self.target_dir = self._first_not_none(
+        self.target_dir = self.target_dir = self._first_not_none(


Typo without side effects but still - let's fix:

Suggested change

self.target_dir = self.target_dir = self._first_not_none(

self.target_dir = self._first_not_none(

RoiTabach · 2023-03-15T12:13:07Z

+        except Exception:
+            pass
+        finally:
+            os.makedirs(os.path.abspath(os.path.abspath(file_path)), exist_ok=True)


we don't need to double abspath , right?

Suggested change

os.makedirs(os.path.abspath(os.path.abspath(file_path)), exist_ok=True)

os.makedirs(os.path.abspath(file_path), exist_ok=True)

RoiTabach · 2023-03-15T12:13:44Z

+LOG_FILE = os.getcwd() + "/edr_files/edr.log"
+
+
+class CustomerFileHandler(logging.FileHandler):


Is this customer and not custom intentionaly? I don't know English well enough to be sure

haritamar · 2023-03-14T13:51:35Z


    DEFAULT_CONFIG_DIR = str(Path.home() / ".edr")

+    DEFAULT_FILES_PATH = os.getcwd() + "/edr_files"


Maybe edr_outputs would be a better name?
Not sure

Or asked for "edr_target" since it's compliant with dbt target dir

haritamar · 2023-03-14T13:52:03Z

+            "-tp",
+            type=str,
+            default=Config.DEFAULT_FILES_PATH,
+            help="Absolut target path for saving edr files such as logs and reports",


Suggested change

help="Absolut target path for saving edr files such as logs and reports",

help="Absolute target path for saving edr files such as logs and reports",

haritamar · 2023-03-14T13:55:50Z

+            target_path_flag = "--target-path"
+            ctx = click.get_current_context()
+            ctx_args = ctx.args
+            if target_path_flag in ctx_args:


Why do we need to check by both the flag and the param?

Because in the context of the cli/cli.py it's an arg, but in the context of some other files it's in the params. I guess its based on if it's before or after the get_cli_properties function @haritamar

haritamar · 2023-03-14T13:56:34Z

+        file_path = os.getcwd() + "/edr_files"
+        try:
+            target_path_param = "target_path"
+            target_path_flag = "--target-path"


What if the user passes it as -tp rather than --target-path

That's a good point
I think I'll remove the -tp, it felt redundant from the start and also kind of confusing with the -t option
Is that ok? @haritamar

haritamar · 2023-03-15T07:50:30Z

+LOG_FILE = os.getcwd() + "/edr_files/edr.log"
+
+
+class CustomerFileHandler(logging.FileHandler):


We normally use the term User rather than Customer in OSS, so let's do that here as well :)

Should have been Custom 😅

haritamar · 2023-03-15T15:02:42Z


 def get_file_handler():
-    file_handler = logging.FileHandler(LOG_FILE)
+    file_handler = CustomFileHandler(LOG_FILE, delay=True)


I think we should add the file handler dynamically with the correct log file output, initializing it with LOG_FILE and then changing it in emit feels a bit hacky...

I think it's better that we'll add a line of the format:
logger.addHandler(get_file_handler(log_path))

to elementary/monitor/cli.py (where log_path is constructed from the target path parameter.

Writing here too -
Basically we're initializing a different logger in every file we have - (Which is a best practice in python),
So we need to somehow from every file to set the file handler to be ontarget_path - at that point I thought about getting the context in the log file itself, but then realized we're initializing it in the imports of the cli.py - which is before they have the context.

Resolution after huddle with Itamar I'll remove the custom handler and use the root logger (remove the "propagate = False") and set it's handler after the context is already set (this can happen even after the rest of the loggers are already initialized and they would be updated)

elongl · 2023-03-15T15:24:16Z

    full_refresh_dbt_package = params.get("full_refresh_dbt_package")

    return {
+        "target_path": target_path,


Is this necessary?
What does it affect?

Are you asking if we need it in the return or if we need the "target_path": target_path instead of just returning target_path itself?

For the first question - I'm pretty sure that's how we set the params (instead of it being in the args)

haritamar · 2023-03-15T17:21:18Z

    return logger
+
+
+def set_root_logger_file_handler(logger_name):


I think you should add both handlers only to the root logger, otherwise you'll get duplicate console outputs (outcome of removing propagate=False).
So maybe we can have set_root_logger_handlers that handles both?

I actually don't see it log twice to the console so it's interesting
But anyway I'll move the console handler to the root as well, I agree it's more sorted that way

haritamar · 2023-03-15T17:24:02Z

        self.format_epilog(ctx, formatter)

    def invoke(self, ctx: click.Context) -> Any:
+        set_root_logger_file_handler("elementary")


I think it may be good that set_root_logger_file_handler will explicitly get the target path rather than relying on the click context.
WDYT about moving the call to config.py below and then doing set_root_logger_file_handler("elementary", target_path)?

I think it makes sense to do it right after you're creating the directory there.

Hmm I see at that point in the code the config still points to the default edr_files path, maybe you can show me why
but I can also get it from the click context in the cli.py instead of the log (unless what you dislike here is the context itself and not the location of it)

haritamar · 2023-03-15T17:30:47Z

    return console_handler


+def get_log_path():


See my note above regarding getting the target_path as a parameter, in that case I think this function can simply be joining it with "edr.log" :)

haritamar · 2023-03-15T17:35:44Z

+        file_path = ctx_args[ctx_args.index(target_path_flag) + 1]
+    finally:
+        os.makedirs(os.path.abspath(file_path), exist_ok=True)
+        return file_path + "/edr.log"


it's a good practice in python usually to use os.path.join(file_path, "edr.log") rather than adding strings
(handles well edge cases, like if the path ends or doesn't end with "/")

Ha good to know, I saw both in the code so just used one of them 👍

haritamar · 2023-03-15T17:36:53Z

        with:
          name: edr.log
-          path: edr.log
+          path: edr_files/edr.log


Should this be changed to edr_target? (also for other cases below)

noaKurman requested a review from elongl March 13, 2023 15:32

noaKurman removed the request for review from elongl March 13, 2023 15:32

noaKurman force-pushed the ele-303-add-target-path branch 3 times, most recently from cfc280b to f0467d8 Compare March 13, 2023 15:59

noaKurman force-pushed the ele-303-add-target-path branch from eb701e7 to dbac505 Compare March 14, 2023 09:12

Maayan-s reviewed Mar 15, 2023

View reviewed changes

noaKurman force-pushed the ele-303-add-target-path branch from dbac505 to 93530e2 Compare March 15, 2023 10:51

noaKurman requested a review from Maayan-s March 15, 2023 11:14

Maayan-s linked an issue Mar 15, 2023 that may be closed by this pull request

[ELE-303] edr should have a target folder for all outputs #613

Closed

RoiTabach approved these changes Mar 15, 2023

View reviewed changes

noaKurman force-pushed the ele-303-add-target-path branch from 9c874fc to c7ac650 Compare March 15, 2023 12:24

haritamar reviewed Mar 15, 2023

View reviewed changes

elongl reviewed Mar 15, 2023

View reviewed changes

noaKurman requested review from elongl and haritamar March 15, 2023 16:22

haritamar reviewed Mar 15, 2023

View reviewed changes

haritamar approved these changes Mar 16, 2023

View reviewed changes

Add target_path flag option for edr files and logs

75bc7e4

noaKurman force-pushed the ele-303-add-target-path branch from e10b1ee to 75bc7e4 Compare March 16, 2023 09:39

noaKurman merged commit ba09d5e into master Mar 16, 2023

noaKurman deleted the ele-303-add-target-path branch March 16, 2023 09:55

	self.target_dir = self.target_dir = self._first_not_none(
	self.target_dir = self._first_not_none(

	os.makedirs(os.path.abspath(os.path.abspath(file_path)), exist_ok=True)
	os.makedirs(os.path.abspath(file_path), exist_ok=True)

		LOG_FILE = os.getcwd() + "/edr_files/edr.log"


		class CustomerFileHandler(logging.FileHandler):


		DEFAULT_CONFIG_DIR = str(Path.home() / ".edr")

		DEFAULT_FILES_PATH = os.getcwd() + "/edr_files"

	help="Absolut target path for saving edr files such as logs and reports",
	help="Absolute target path for saving edr files such as logs and reports",

Conversation

noaKurman commented Mar 13, 2023

Uh oh!

linear bot commented Mar 13, 2023

Uh oh!

github-actions bot commented Mar 13, 2023

Uh oh!

RoiTabach commented Mar 13, 2023

Uh oh!

Maayan-s commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noaKurman commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maayan-s Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RoiTabach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haritamar Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haritamar Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Maayan-s commented Mar 15, 2023 •

edited

Loading

noaKurman commented Mar 15, 2023 •

edited

Loading

Maayan-s Mar 15, 2023 •

edited

Loading

haritamar Mar 15, 2023 •

edited

Loading

haritamar Mar 15, 2023 •

edited

Loading