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

Handle syslog writing from the command line #9513

Merged
merged 41 commits into from
Nov 5, 2021
Merged

Handle syslog writing from the command line #9513

merged 41 commits into from
Nov 5, 2021

Conversation

PaulNewtech
Copy link
Contributor

Proposed changes:

  • This change allows you to give the system log handler as a parameter to the rasa run command line

@PaulNewtech PaulNewtech requested a review from a team as a code owner September 2, 2021 09:48
@PaulNewtech PaulNewtech requested review from joejuzl and removed request for a team September 2, 2021 09:48
@CLAassistant
Copy link

CLAassistant commented Sep 2, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

joejuzl
joejuzl suggested changes Sep 7, 2021
View reviewed changes
Copy link
Contributor

@joejuzl joejuzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this submission!
Added a couple of comments.

rasa/core/utils.py Outdated Show resolved Hide resolved
rasa/core/utils.py Outdated Show resolved Hide resolved
rasa/core/utils.py Show resolved Hide resolved
@PaulNewtech
Copy link
Contributor Author

Hello joejuzl,
Thank you for the review. I took your comments into account. Now the syslog is directly handled by a new arg in the command line.
I look forward to see what you think about this new version.

@PaulNewtech PaulNewtech requested a review from joejuzl September 15, 2021 10:20
joejuzl
joejuzl suggested changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@joejuzl joejuzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Thanks for making those changes, I think the CLI flag makes sense.
Can you add a changelog please?

Also, the CI will now run, so you may have to fix some tests and code style issues.

rasa/core/utils.py Outdated Show resolved Hide resolved
rasa/utils/common.py Outdated Show resolved Hide resolved
@PaulNewtech
Copy link
Contributor Author

Hi Joe,

I just added a few things to the pull request.

As you said the use of "/dev/log" was a problem with system compatibility. Using the UDP communication with the local syslog server is the way you can manage this OS issue.
That's why I decided to had the capability to specify which server, and port your syslog server is listening on and the protocol that is used. There is now those 3 flag more.

It will be very useful for production purposes.

I also manage a few pipeline error.

I will do the changelog after your response :)

Looking forward to reading from you.

@PaulNewtech PaulNewtech requested a review from joejuzl September 17, 2021 09:33
@joejuzl
Copy link
Contributor

joejuzl commented Sep 23, 2021

Hi @PaulNewtech
It looks like some CI checks are failing.
You will need to make sure that Code Quality passes and all the tests.
I will approve and run the workflow, make sure to check the job statuses before merging master again so you can see what needs to be fixed.
Let me know if you need any help.

@PaulNewtech
Copy link
Contributor Author

Hi @joejuzl ,

I hope it will be good with this fix.
But the error log of the test are not very explicit.

@joejuzl
Copy link
Contributor

joejuzl commented Sep 23, 2021

The code quality test is failing. Try running make lint to check locally. (hint: you will need to run the black formatter on your code)

Also for tests you can run make test see: https://github.com/RasaHQ/rasa#running-the-tests

@joejuzl
Copy link
Contributor

joejuzl commented Oct 29, 2021

Hey @PaulNewtech do you think you'll find time to finalise this?

@PaulNewtech
Copy link
Contributor Author

Hi @joejuzl
I did the required change for the black linter.
The code climate issue to fix seems to have nothing to do with my changes (it was more than 25 lines before it)

@joejuzl
Copy link
Contributor

joejuzl commented Nov 2, 2021

Please make sure to run make lint to check all the linting steps pass.

The code climate issue to fix seems to have nothing to do with my changes (it was more than 25 lines before it)

Codeclimate is not a required check right now - so that's ok.

@PaulNewtech
Copy link
Contributor Author

Thank you !
It's done

@PaulNewtech
Copy link
Contributor Author

Hey @joejuzl ,
I just made changes to the tests to be compliant with the new CLI flags.
Thank you

joejuzl
joejuzl approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@joejuzl joejuzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 🚀 🚀

@joejuzl joejuzl merged commit b773f71 into RasaHQ:main Nov 5, 2021
@joejuzl
Copy link
Contributor

joejuzl commented Nov 5, 2021

Thanks for putting in all the work to get it merged! 🔥

@PaulNewtech
Copy link
Contributor Author

Thank you @joejuzl !

srinivasupadhya pushed a commit to srinivasupadhya/rasa that referenced this pull request Nov 8, 2021
@PaulNewtech @joejuzl @srinivasgoto
Handle syslog writing from the command line (RasaHQ#9513)
f4a2bc1 
* Handle syslog writing from the command line

* add an arg in the command line to use syslog

* respect standards

* line was too long

* new arg for command line

* add sanic logs to syslog

* del useless statement

* quick fix

* too many blank line

* Use default communication with syslog

* possibility to use a syslog server

* fix indent

* Fix for constants access

* fix typo

Co-authored-by: Joe Juzl <joejuzl@gmail.com>

* fix typo

* fix for constant definition in bad file

* Black linter compliant

* Function add_server_arguments had 26 lines of code (exceeds 25 allowed).

* fix for bad merge

* for lint compliance

* add required param

* adapt test for log enhancement

Co-authored-by: Joe Juzl <joejuzl@gmail.com>
indam23 pushed a commit that referenced this pull request Jul 27, 2022
@PaulNewtech @joejuzl
Handle syslog writing from the command line (#9513)
4e30494 
* Handle syslog writing from the command line

* add an arg in the command line to use syslog

* respect standards

* line was too long

* new arg for command line

* add sanic logs to syslog

* del useless statement

* quick fix

* too many blank line

* Use default communication with syslog

* possibility to use a syslog server

* fix indent

* Fix for constants access

* fix typo

Co-authored-by: Joe Juzl <joejuzl@gmail.com>

* fix typo

* fix for constant definition in bad file

* Black linter compliant

* Function add_server_arguments had 26 lines of code (exceeds 25 allowed).

* fix for bad merge

* for lint compliance

* add required param

* adapt test for log enhancement

Co-authored-by: Joe Juzl <joejuzl@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joejuzl joejuzl joejuzl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PaulNewtech @CLAassistant @joejuzl