-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Added logging device and logging device options #41416
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.
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
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
|
We might need to add some unit tests to this change as well |
|
@Lee-W I'm not quite sure what kind of unit tests would be valid here, since this is an operator update. The operator doesn't return anything to check. I do have a test DAG if that counts, but you would need to have Docker Swarm and Graylog setup to test. |
…tests for logging driver validity
|
@Lee-W |
…tests for logging driver validity
Lee-W
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good. The only remaining issue is one small problem, and we might need to add "gelf" to the spell check valid word, as it's not recognized and fails the CI.
Lee-W
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are still some minor issues. You can run pre-commit locally and get them resolved. I think we're really close to merge 👍
|
@Lee-W I have update the |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Seems that apache#41416 PR has been somehow (?) deleted and when we try to generate issue links for it, the issue generation fails (and it is not an issue as well). This change should workaround that by skipping the PR.
Seems that #41416 PR has been somehow (?) deleted and when we try to generate issue links for it, the issue generation fails (and it is not an issue as well). This change should workaround that by skipping the PR.
Closes: #40533
Updated the DockerSwarmOperator with logging device and logging device options parameters as requested. I restricted support for only 'json-file' and 'gelf', since that is all I was able to test locally. I can include support for other logging drivers (e.g. fluentd) if it makes sense. Also, I wasn't sure if I should have a check in place for valid logging options. Happy to explore if it makes sense. Here is some info on the GELF logging driver and its options, https://docs.docker.com/engine/logging/drivers/gelf/.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.