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 2022/06/26 17:54:37 UTC
[GitHub] [airflow] mayankbatra123 opened a new pull request, #24668: added component specific labels
mayankbatra123 opened a new pull request, #24668:
URL: https://github.com/apache/airflow/pull/24668
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: [#ISSUE](https://github.com/apache/airflow/discussions/23828)
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ 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 a newsfragement file, named `{pr_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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] jedcunningham commented on pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on PR #24668:
URL: https://github.com/apache/airflow/pull/24668#issuecomment-1224524580
Closing this as #25013 has added this feature. Thanks @mayankbatra123!
--
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] jedcunningham commented on a diff in pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24668:
URL: https://github.com/apache/airflow/pull/24668#discussion_r938307622
##########
chart/templates/cleanup/cleanup-cronjob.yaml:
##########
@@ -38,9 +38,9 @@ metadata:
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
-{{- with .Values.labels }}
-{{ toYaml . | indent 4 }}
-{{- end }}
+ {{- if or (.Values.labels) (.Values.cleanup.labels) }}
+ {{- mustMerge .Values.cleanup.labels .Values.labels | toYaml | nindent 4 }}
+ {{- end }}
Review Comment:
No, thinking again merging is just fine.
--
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 #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24668:
URL: https://github.com/apache/airflow/pull/24668#issuecomment-1174374724
This one has static chceks and docs failing. You need to fix it before we review it @mayankbatra123
--
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] mayankbatra123 commented on a diff in pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
mayankbatra123 commented on code in PR #24668:
URL: https://github.com/apache/airflow/pull/24668#discussion_r944604519
##########
chart/templates/cleanup/cleanup-cronjob.yaml:
##########
@@ -38,9 +38,9 @@ metadata:
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
-{{- with .Values.labels }}
-{{ toYaml . | indent 4 }}
-{{- end }}
+ {{- if or (.Values.labels) (.Values.cleanup.labels) }}
+ {{- mustMerge .Values.cleanup.labels .Values.labels | toYaml | nindent 4 }}
+ {{- end }}
Review Comment:
I think it makes sense to include both labels global level and component specific.
This would give more flexibility to the user. All generic labels can be added at global level and specific ones can be added to each component.
--
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] jedcunningham closed pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #24668: added component specific labels
URL: https://github.com/apache/airflow/pull/24668
--
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] jedcunningham commented on a diff in pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24668:
URL: https://github.com/apache/airflow/pull/24668#discussion_r911456168
##########
chart/Chart.yaml:
##########
@@ -19,7 +19,7 @@
---
apiVersion: v2
name: airflow
-version: 1.7.0-dev
+version: 1.8.0-dev
Review Comment:
```suggestion
version: 1.7.0-dev
```
This should still be 1.7.0-dev. This gets bumped as part of the release process.
##########
chart/templates/cleanup/cleanup-cronjob.yaml:
##########
@@ -38,9 +38,9 @@ metadata:
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
-{{- with .Values.labels }}
-{{ toYaml . | indent 4 }}
-{{- end }}
+ {{- if or (.Values.labels) (.Values.cleanup.labels) }}
+ {{- mustMerge .Values.cleanup.labels .Values.labels | toYaml | nindent 4 }}
+ {{- end }}
Review Comment:
I wonder if we should follow the lead of tolerations and topologySpreadConstraints and do `or .Values.cleanup.labels .Values.labels` instead of merging.
--
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] jedcunningham commented on a diff in pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on code in PR #24668:
URL: https://github.com/apache/airflow/pull/24668#discussion_r938319188
##########
chart/values.yaml:
##########
@@ -583,6 +585,8 @@ workers:
# Airflow scheduler settings
scheduler:
+ # Label Specific to scheduler app
Review Comment:
```suggestion
# Labels specific to scheduler objects and pods
```
And here.
##########
chart/values.schema.json:
##########
@@ -1450,6 +1450,14 @@
"type": "string"
}
},
+ "labels": {
+ "description": "Labels to add to the worker pods.",
Review Comment:
```suggestion
"description": "Labels to add to the worker objects and pods.",
```
We apply these to all worker objects, not just the pod. This matches what we have for `labels` currently.
--
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] rishkarajgi commented on pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
rishkarajgi commented on PR #24668:
URL: https://github.com/apache/airflow/pull/24668#issuecomment-1172310348
@mayankbatra123 Is this PR feature complete?
@jedcunningham can we expedite this for an upcoming astronomer feature?
--
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 #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on PR #24668:
URL: https://github.com/apache/airflow/pull/24668#issuecomment-1166600935
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 commented on pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #24668:
URL: https://github.com/apache/airflow/pull/24668#issuecomment-1172641527
@mayankbatra123 - I think everything we work on here is community Airflow.
If you want to discuss with astronomer on adding stuff for Astronomer, this is not the right forum (and this one is wrong). I am not even sure what you are talking about here.
--
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 #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
uranusjr commented on PR #24668:
URL: https://github.com/apache/airflow/pull/24668#issuecomment-1217353637
This one’s pretty broken
--
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] rishkarajgi commented on a diff in pull request #24668: added component specific labels
Posted by GitBox <gi...@apache.org>.
rishkarajgi commented on code in PR #24668:
URL: https://github.com/apache/airflow/pull/24668#discussion_r918799436
##########
chart/templates/cleanup/cleanup-cronjob.yaml:
##########
@@ -38,9 +38,9 @@ metadata:
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
release: {{ .Release.Name }}
heritage: {{ .Release.Service }}
-{{- with .Values.labels }}
-{{ toYaml . | indent 4 }}
-{{- end }}
+ {{- if or (.Values.labels) (.Values.cleanup.labels) }}
+ {{- mustMerge .Values.cleanup.labels .Values.labels | toYaml | nindent 4 }}
+ {{- end }}
Review Comment:
@jedcunningham I tested this by adding labels to both global labels and to container specific labels. It is showing both in the helm template.
An alternative to merge would be using two if conditions
--
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