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/06/24 17:28:18 UTC
[GitHub] [airflow] SimonOsipov opened a new pull request #16636: Added dashes to pod naming
SimonOsipov opened a new pull request #16636:
URL: https://github.com/apache/airflow/pull/16636
Closes: #16600
---
In order to have a more readable naming of KubernetesExecutor Pod, decided to remove deleting "dashes" as per the [documentation](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/) it is allowed.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-913058279
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-868212846
> It seems like already provided test cases are enough
Some more should be added since the logic now has more allowed characters. Name with leading dash/dot/underscore, for example.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SimonOsipov commented on a change in pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
SimonOsipov commented on a change in pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#discussion_r658156475
##########
File path: airflow/kubernetes/kubernetes_helper_functions.py
##########
@@ -29,15 +29,13 @@ def _strip_unsafe_kubernetes_special_chars(string: str) -> str:
"""
Kubernetes only supports lowercase alphanumeric characters, "-" and "." in
the pod name.
- However, there are special rules about how "-" and "." can be used so let's
- only keep
- alphanumeric chars see here for detail:
+ See here for detail:
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
:param string: The requested Pod name
:return: Pod name stripped of any unsafe characters
"""
- return ''.join(ch.lower() for ch in list(string) if ch.isalnum())
+ return ''.join(ch.lower() for ch in list(string) if ch.isalnum() or ch in ['-'])
Review comment:
Can you elaborate more on why using an additional module would be more effective? I am fairly new to Python, so I am eager for knowledge.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-949090655
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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 pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-883822361
#17057 only covers the `kubernetes_helper_functions.py` part of this PR; we still need other parts of this.
--
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] SimonOsipov commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
SimonOsipov commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-913727880
I terribly apologize, I a bit felt out of the issue I created. Is this issue still valid?
--
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] SimonOsipov commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
SimonOsipov commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-867842512
> Some test cases will also be needed.
https://github.com/apache/airflow/blob/86d0a96bf796fd767cf50a7224be060efa402d94/tests/kubernetes/test_pod_generator.py#L650-L679
It seems like already provided test cases are enough (they are with dashes). Also, regex is valid for using dashes.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-883820693
I think #17057 covers this.
--
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 closed pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
uranusjr closed pull request #16636:
URL: https://github.com/apache/airflow/pull/16636
--
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] SimonOsipov commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
SimonOsipov commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-880177778
I am sorry, have been sick lately, can we return to this MR?
What was the decision and what additional things needs to be done to implement this (I would like to take and complete them) =)
@uranusjr @kaxil @houqp
--
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] houqp commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
houqp commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-880313628
@SimonOsipov i think you need to write code to check and remove trailing `-` after the truncation so when we concat them with `.`, it won't result in invalid pod name in k8s. And of course add some unit tests for it :)
--
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 #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#discussion_r658151916
##########
File path: airflow/kubernetes/kubernetes_helper_functions.py
##########
@@ -29,15 +29,13 @@ def _strip_unsafe_kubernetes_special_chars(string: str) -> str:
"""
Kubernetes only supports lowercase alphanumeric characters, "-" and "." in
the pod name.
- However, there are special rules about how "-" and "." can be used so let's
- only keep
- alphanumeric chars see here for detail:
+ See here for detail:
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
:param string: The requested Pod name
:return: Pod name stripped of any unsafe characters
"""
- return ''.join(ch.lower() for ch in list(string) if ch.isalnum())
+ return ''.join(ch.lower() for ch in list(string) if ch.isalnum() or ch in ['-'])
Review comment:
It’s probably easier to do a `re.sub` instead.
##########
File path: airflow/kubernetes/kubernetes_helper_functions.py
##########
@@ -52,7 +50,7 @@ def create_pod_id(dag_id: str, task_id: str) -> str:
"""
safe_dag_id = _strip_unsafe_kubernetes_special_chars(dag_id)
safe_task_id = _strip_unsafe_kubernetes_special_chars(task_id)
- return safe_dag_id + safe_task_id
+ return safe_dag_id + "-" + safe_task_id
Review comment:
```suggestion
return f"{safe_dag_id}-{safe_task_id}"
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#discussion_r658481898
##########
File path: airflow/kubernetes/kubernetes_helper_functions.py
##########
@@ -29,15 +29,13 @@ def _strip_unsafe_kubernetes_special_chars(string: str) -> str:
"""
Kubernetes only supports lowercase alphanumeric characters, "-" and "." in
the pod name.
- However, there are special rules about how "-" and "." can be used so let's
- only keep
- alphanumeric chars see here for detail:
+ See here for detail:
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
:param string: The requested Pod name
:return: Pod name stripped of any unsafe characters
"""
- return ''.join(ch.lower() for ch in list(string) if ch.isalnum())
+ return ''.join(ch.lower() for ch in list(string) if ch.isalnum() or ch in ['-'])
Review comment:
Not many reasons TBH, just it’s probably easier to understand IMO.
```python
return re.sub(r"[^-a-z0-9]+", string.lower())
```
It may also be easier to implement additional logic, such as avoiding `-` in the beginning of the string (which the logic you implement right now does not do).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-867820570
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, pylint 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] houqp commented on a change in pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#discussion_r665076303
##########
File path: airflow/kubernetes/kubernetes_helper_functions.py
##########
@@ -29,15 +29,13 @@ def _strip_unsafe_kubernetes_special_chars(string: str) -> str:
"""
Kubernetes only supports lowercase alphanumeric characters, "-" and "." in
the pod name.
- However, there are special rules about how "-" and "." can be used so let's
- only keep
- alphanumeric chars see here for detail:
+ See here for detail:
https://kubernetes.io/docs/concepts/overview/working-with-objects/names/
:param string: The requested Pod name
:return: Pod name stripped of any unsafe characters
"""
- return ''.join(ch.lower() for ch in list(string) if ch.isalnum())
+ return ''.join(ch.lower() for ch in list(string) if ch.isalnum() or ch in ['-'])
Review comment:
if we are to use regex, we should use a precompiled regex object. If we are not going to use regex here, then i think we should just do the comparison with `ch == '-'` without creating a new list in memory.
--
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] kaxil commented on pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #16636:
URL: https://github.com/apache/airflow/pull/16636#issuecomment-868869024
There were also https://github.com/apache/airflow/pull/15443 and https://github.com/apache/airflow/pull/15445
'-' followed by '.' isn't allowed which caused issues when pod_id was trimmed
cc @houqp
--
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] closed pull request #16636: Added dashes to pod naming
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #16636:
URL: https://github.com/apache/airflow/pull/16636
--
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