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

Jump to bottom

Ignore the log files when they are generated #236

Closed
wants to merge 3 commits into from
Closed

Ignore the log files when they are generated #236

wants to merge 3 commits into from

Conversation

thombet
Copy link

@thombet thombet commented May 2, 2020 •
edited

Fixes #233

  • Create two functions in the common module to get the full path of the
    log files (debug log and reporter log) anywhere in the script. It
    avoids duplicate reference to the names of the log files to keep the code DRY.
  • When a log file is generated it will be whitelisted to avoid reporting warnings.
  • Files with the same name as the log files but not in the current folder will still be reported (these files were generated from a previous run of the checker so they should not be ignored)

@thombet
Copy link
Author

thombet commented May 2, 2020 •
edited

Some additional information:

  • It seems both .format() and % are used in the code but I used % in __main___.py to be consistent with the rest of this file.
  • From a design point of view I think it would have been better to define get_reporter_log_path() in the LogReporter class (plugins/log_reporter.py) but I didn't find a good way to use this class accross the code since it is imported with load_plugins() in __main__.py. If you have a better idea, feel free to suggest it :)
  • I ran several (manual) tests locally to confirm it is working as expected but should I create any auto tests? See new commits below

@thombet thombet mentioned this pull request May 2, 2020
Open
@thombet
Copy link
Author

thombet commented May 4, 2020

The tests failed because the stub of argParser does not work anymore with my change.
I will fix this and probably add some tests to reproduce the use cases I tested manually.

@thombet
Ignore the log files when they are generated
1b7cd56 
Create two functions in the common module to get the full path of the
log files (debug log and reporter log) anywhere in the script. It will
avoid duplicate reference to the names of the log files.
Then whitelist the log file(s) generated to avoid reporting warnings.
Note that log files generated from another run will still be reported.
@thombet
Create new test cases to validate the new feature
2a16084 
Fix the existing tests by adding the arguments "reporter" and
"enable_debug_log" in the stub of argparser (Args()).
Create new test cases to validate the log files are correctly ignored
or reported.

Also improve a bit the code in check_file_whitelist: check if the
file needs to be ignored at the very beginning of the loop since the
the other check (using regex) requires more lines to be executed.
@thombet
Fix pylint errors
9c52179
@enen92
Copy link
Member

enen92 commented May 31, 2020 •
edited

There are a lot of use cases for this tool, namely via pipelines. I am not sure I like the idea of ignoring specific files that may end up bundled with the addon submission.
Maybe a possible option is to only allow usage of --log or --debug log if an additional argument (log path) is submitted?
Maybe also error out if the submitted log path is equal to the directory we are checking for validation?

Another option is to make it platform dependent and make it log by default to the standard log location per platform (/var/log/addon-check on linux, ~/Library/Logs/ on osx, etc) and at the end of the execution state the log can be found in xxxx ?

@thombet
Copy link
Author

thombet commented Jun 6, 2020

I came up with this solution because I thought requesting a path for the log files would break the compatibility with previous versions but your idea of using a default path solves this 🙂

I agree this PR adds a risk of submitting ignored files when used in pipelines so I will push a new update implementing your ideas. Thank you for suggesting them @enen92 👍

@razzeee
Copy link
Member

razzeee commented Sep 20, 2020

Should we close this until you find time to work on this again?

@thombet
Copy link
Author

thombet commented Sep 28, 2020

Should we close this until you find time to work on this again?

I can't believe it's been 4 months already...
Yes I will close this pull request and I will upload a new one later, thanks for the reminder.

@thombet thombet closed this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Log file generates a warning when --reporter=log is used
3 participants
@thombet @enen92 @razzeee