You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/07/13 19:21:46 UTC
[GitHub] [airflow] BShraman opened a new pull request #16978: Updated clean-logs.sh
BShraman opened a new pull request #16978:
URL: https://github.com/apache/airflow/pull/16978
<!--
Declared local variable to readonly.
Added "<" for integer comparison
Updated "[[]]" ro "(())"for numerical comparison
-->
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669310831
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -33,5 +32,5 @@ while true; do
find "${DIRECTORY}"/logs -mtime +"${RETENTION}" -name '*.log' -delete
seconds=$(( $(date -u +%s) % EVERY))
- [[ $seconds -lt 1 ]] || sleep $((EVERY - seconds))
+ (( "${seconds}" < 1 )) || sleep $((EVERY - seconds))
Review comment:
Actually I like the new way better. Explicit math bash expression is more readable than -lt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BShraman edited a comment on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
BShraman edited a comment on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-879866317
Hi @potiuk , i have followed below steps before push to GIT am i missing anything ? Could you please let me know since this is my first push.
-pre-commit run --files scripts/in_container/prod/clean-logs.sh
-pre-commit install
-git commit -m "Updated clean-logs.sh"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BShraman commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
BShraman commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-879866317
Hi @potiuk , i have followed below steps before push to GIT am i missing anything ? Could you please let me know since this is my first push.
-pre-commit run --files scripts/in_container/prod/clean-logs.sh
-pre-commit install
- git commit -m "Updated clean-logs.sh"
-
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BShraman edited a comment on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
BShraman edited a comment on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-880699901
Hi @potiuk @uranusjr anything else on this change? Since this is my first push , I wanted to be learn the process before i can start on next task.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669310429
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -17,14 +16,14 @@
# specific language governing permissions and limitations
# under the License.
-set -euo pipefail
+set -e pipefail
Review comment:
It won't work by the way :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BShraman commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
BShraman commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-879876494
Sure , let me add on my steps for future push.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669078131
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -33,5 +32,5 @@ while true; do
find "${DIRECTORY}"/logs -mtime +"${RETENTION}" -name '*.log' -delete
seconds=$(( $(date -u +%s) % EVERY))
- [[ $seconds -lt 1 ]] || sleep $((EVERY - seconds))
+ (( "${seconds}" < 1 )) || sleep $((EVERY - seconds))
Review comment:
Why is the suggested syntax considered better than the current one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-879614836
And please run it through `pre-commit` before. We use `shell-check` and it should flag some of the changes you made as wrong
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669312282
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -33,5 +32,5 @@ while true; do
find "${DIRECTORY}"/logs -mtime +"${RETENTION}" -name '*.log' -delete
seconds=$(( $(date -u +%s) % EVERY))
- [[ $seconds -lt 1 ]] || sleep $((EVERY - seconds))
+ (( "${seconds}" < 1 )) || sleep $((EVERY - seconds))
Review comment:
However it should be simpler:
```suggestion
(( seconds < 1 )) || sleep $((EVERY - seconds))
```
in math expression there is no need for neither quoting nor '$'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BShraman commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
BShraman commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669570559
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -33,5 +32,5 @@ while true; do
find "${DIRECTORY}"/logs -mtime +"${RETENTION}" -name '*.log' -delete
seconds=$(( $(date -u +%s) % EVERY))
- [[ $seconds -lt 1 ]] || sleep $((EVERY - seconds))
+ (( "${seconds}" < 1 )) || sleep $((EVERY - seconds))
Review comment:
Suggested by google cheat-sheet for numerical comparison and for better and simpler understanding
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BShraman commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
BShraman commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669585312
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -17,14 +16,14 @@
# specific language governing permissions and limitations
# under the License.
-set -euo pipefail
+set -e pipefail
Review comment:
I reverted by changes and pushed by code again. I was missing pre-commit test in my first push.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669312282
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -33,5 +32,5 @@ while true; do
find "${DIRECTORY}"/logs -mtime +"${RETENTION}" -name '*.log' -delete
seconds=$(( $(date -u +%s) % EVERY))
- [[ $seconds -lt 1 ]] || sleep $((EVERY - seconds))
+ (( "${seconds}" < 1 )) || sleep $((EVERY - seconds))
Review comment:
However it should be simpler:
```suggestion
(( seconds < 1 )) || sleep $((EVERY - seconds))
```
in math expression there is no need for either quoting or '$'
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-879868525
Should be ok. I usually also rebase my commits on top of `main` (see CONTRIBUTING - we have description about rebasing).
This will automatically account for any changes from `main` that should be incorporated (it happens occasionally that they need to be).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669309829
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -17,14 +16,14 @@
# specific language governing permissions and limitations
# under the License.
-set -euo pipefail
+set -e pipefail
Review comment:
Yeah... why ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669076612
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -17,14 +16,14 @@
# specific language governing permissions and limitations
# under the License.
-set -euo pipefail
+set -e pipefail
Review comment:
Why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-881196075
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-881065532
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-879339519
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #16978:
URL: https://github.com/apache/airflow/pull/16978
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] BShraman commented on pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
BShraman commented on pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#issuecomment-880699901
Hi @potiuk @uranusjr anything else on this change? I wanted to be learn the process before i can start on next task.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #16978: Updated clean-logs.sh
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16978:
URL: https://github.com/apache/airflow/pull/16978#discussion_r669310831
##########
File path: scripts/in_container/prod/clean-logs.sh
##########
@@ -33,5 +32,5 @@ while true; do
find "${DIRECTORY}"/logs -mtime +"${RETENTION}" -name '*.log' -delete
seconds=$(( $(date -u +%s) % EVERY))
- [[ $seconds -lt 1 ]] || sleep $((EVERY - seconds))
+ (( "${seconds}" < 1 )) || sleep $((EVERY - seconds))
Review comment:
Actually I like it better. Explicit math bash expression is more readable than -lt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org