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.

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

Adding podAnnotations to StatsD deployment template #25732

Conversation

joshuaghezzi
Copy link
Contributor

Adding PodAnnotation to StatsD deployment template.
Closes: #25446

@boring-cyborg
Copy link

boring-cyborg bot commented Aug 15, 2022

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 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 Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment 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 for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    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

@sbrandtb
Copy link
Contributor

Please, next time you are working on an issue that is assigned to someone else, please notify the issue. I do not want to sound rude, but it is simple fact that I just wasted time on this because I implemented it as well. Time that could have been spent more wisely.

@potiuk
Copy link
Member

potiuk commented Aug 17, 2022

Please, next time you are working on an issue that is assigned to someone else, please notify the issue. I do not want to sound rude, but it is simple fact that I just wasted time on this because I implemented it as well. Time that could have been spent more wisely.

I think the problem was .... you were not assigned @sbrandtb :). It was likely an omission. When you read all the conversation it looked like you wanted to work on it , but yeah - it's our fault we have not assigned you.

BTW. I think it's quite expected that there is some duplication of work. There is also a value in doing stuff (learning). Merging is not the ultimate and only value, the process of making PR. getting reviews and feedback is valuable as well. There is no sane way to make sure that there is no duplication of work, but when you realise this is not the only value, you get more acceptance of doing stuff in parallel by different people. Code is often liability and if you get others contribute the code you came with as an idea, this is actually cool opportunity to work on it together.

Since this PR is already out - in the spirit of what I wrote above - I'd encourage you @sbrandtb to turn into reviewer here and make your comments and help with leading that to completion. There is even bigger value in being reviewer when such code is contributed - you learn how to communicate your review comments, you learn how to look at the change with other perspective and you will likely notice things that you wouldn't have noticed when you wrote it. And a good learning experience too.

At the end "Community over code" is what matters most (this is the ASF motto).

@potiuk
Copy link
Member

potiuk commented Aug 17, 2022

Ah I see you de-assigned yourself (my mistake) - so yeah, probably asking where you stand would be cool. but that does not change my proposal regardless.

@sbrandtb
Copy link
Contributor

@potiuk As mentioned in the issue: No offense. And MR looks good to me, but I do not have the power to +1

@joshuaghezzi joshuaghezzi requested review from jedcunningham and removed request for dstandish September 14, 2022 02:41
@jedcunningham jedcunningham merged commit 951b708 into apache:main Sep 15, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 15, 2022

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm Chart: Allow adding annotations to statsd deployment
4 participants