Adjust trimmed_pod_id and replace '.' with '-' #19036

kd2718 · 2021-10-17T17:41:58Z

closes: #16611
related: #16980

Having a period in the name could cause issues with the hostname. Reverting to hyphen will should fix the issues. I also included changes talked about in a previous PR.

boring-cyborg · 2021-10-17T17:42:02Z

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

uranusjr

cc @dimon222 to take a look if possible

airflow/kubernetes/pod_generator.py

github-actions · 2021-10-19T05:27:55Z

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.

kd2718 · 2021-10-20T15:45:31Z

I modified a few tests to correctly work with the new - syntax. Previous tests tried to split strings looking for ..

airflow/kubernetes/pod_generator.py

kd2718 · 2021-10-20T15:54:47Z

Thanks, I can't spell

kd2718 · 2021-10-25T16:47:32Z

What can I do now to help this along?

dimon222 · 2021-10-25T18:42:41Z

Looks okay to me.

kd2718 · 2021-10-29T00:53:41Z

All tests passing

kd2718 · 2021-11-01T19:02:07Z

hoping this is good to go...

kd2718 · 2021-11-03T22:40:30Z

please re-run tests

tests/kubernetes/test_pod_generator.py

airflow/kubernetes/pod_generator.py

uranusjr · 2021-11-15T08:09:43Z

tests/kubernetes/test_pod_generator.py

Maybe add a comment to explain why 63 and 33 are significant?

I added a few notes explaining where these numbers came from.

uranusjr

Mostly ready but one nitpick

tests/kubernetes/test_pod_generator.py

jedcunningham · 2021-11-15T15:45:16Z

tests/kubernetes/test_pod_generator.py

Suggested change

if len(pod_id) <= 63 - 33:

assert len(parts[0]) == len(pod_id)

else:

assert len(parts[0]) <= 63

assert parts[0] == pod_id[:30]

Did this get lost in the shuffle?

chagned to assert len(parts[0]) <= 30. The prefix can be less than 30, but not larger.

kd2718 · 2021-11-17T06:11:29Z

My last test run, I was getting failures from ./tests/jobs/test_scheduler_job.py, but only for certian db and python combinations. If it fails again, I might need some help as to why. Running it with breeze locally, all the tests from that test file.

kd2718 · 2021-11-19T05:33:31Z

I am getting errors like this one now: https://github.com/apache/airflow/runs/4248182868?check_suite_focus=true#step:7:2000

I am not sure where to go to debug this. Any suggestions? I will see if I can trace it on my own a bit later, but if anyone can point me in the right direction, that would be great.

uranusjr · 2021-11-19T08:01:30Z

There was an error in main that fails Helm tests. Please rebase the branch against latest main.

airflow/kubernetes/pod_generator.py

kd2718 · 2021-11-29T05:45:44Z

There was an error in main that fails Helm tests. Please rebase the branch against latest main.

Do you want to to squash as well?

…ll fail... investigating

bad spelling Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

I added a mock, but it wasn't needed. I think it was also causing some test failure

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

jedcunningham · 2021-11-29T21:54:15Z

No need to squash if you don't want to - we do that automatically when we merge.

boring-cyborg · 2021-11-30T05:00:08Z

Awesome work, congrats on your first merged pull request!

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

kd2718 requested a review from dimberman as a code owner October 17, 2021 17:41

boring-cyborg bot added the provider:cncf-kubernetes Kubernetes (k8s) provider related issues label Oct 17, 2021

uranusjr reviewed Oct 18, 2021

View reviewed changes

airflow/kubernetes/pod_generator.py Outdated Show resolved Hide resolved

uranusjr approved these changes Oct 19, 2021

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Oct 19, 2021

uranusjr reviewed Oct 20, 2021

View reviewed changes

airflow/kubernetes/pod_generator.py Outdated Show resolved Hide resolved

uranusjr approved these changes Oct 20, 2021

View reviewed changes

dimon222 mentioned this pull request Nov 9, 2021

Change pod id notation back to hyphen #16980

Closed

jedcunningham reviewed Nov 9, 2021

View reviewed changes

uranusjr reviewed Nov 15, 2021

View reviewed changes

uranusjr approved these changes Nov 15, 2021

View reviewed changes

jedcunningham reviewed Nov 15, 2021

View reviewed changes

uranusjr approved these changes Nov 17, 2021

View reviewed changes

jedcunningham approved these changes Nov 19, 2021

View reviewed changes

airflow/kubernetes/pod_generator.py Outdated Show resolved Hide resolved

kd2718 added 3 commits November 28, 2021 21:46

replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

25ff373

added changes for PR

b06e4d5

fixed typo and updated tests to use new seperator. Some tests may sti…

1e702ee

…ll fail... investigating

kd2718 and others added 15 commits November 28, 2021 21:46

Update airflow/kubernetes/pod_generator.py

7d9ddc3

bad spelling Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

added space for proper linting

598df6e

fixed more tests

aa5d8c0

removed mock that was not being used

7039d82

I added a mock, but it wasn't needed. I think it was also causing some test failure

finally passing all tests locally

642522a

fixed a few formatting errors

c418815

fixed test max length from github comments

e66b9b4

:qUpdate tests/kubernetes/test_pod_generator.py

2b635ba

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

Update tests/kubernetes/test_pod_generator.py

a1b4e5e

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

Update tests/kubernetes/test_pod_generator.py

384b1d3

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

Update airflow/kubernetes/pod_generator.py

d4acfb7

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

removed join and replaced with rsplit(-,1)

67f2f30

cleaned up test_pod_name_confirm_to_max_length as noted by comments

7023fc6

fixed spelling error on separating

724807f

returned directly from function rather than creating a new variable

c1f0796

Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>

kd2718 force-pushed the iss-16611 branch from 6883b06 to c1f0796 Compare November 29, 2021 05:51

uranusjr changed the title ~~replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments~~ Adjust trimmed_pod_id and replace '.' with '-' Nov 30, 2021

uranusjr merged commit 5633158 into apache:main Nov 30, 2021

jedcunningham added the type:bug-fix Changelog: Bug Fixes label Apr 7, 2022

dimon222 mentioned this pull request Apr 13, 2022

Fix KPO to have hyphen instead of period #22982

Merged

-        if len(pod_id) <= 63 - 33:
-            assert len(parts[0]) == len(pod_id)
-        else:
-            assert len(parts[0]) <= 63
+        assert parts[0] == pod_id[:30]

Adjust trimmed_pod_id and replace '.' with '-' #19036

Adjust trimmed_pod_id and replace '.' with '-' #19036

Uh oh!

Conversation

kd2718 commented Oct 17, 2021

Uh oh!

boring-cyborg bot commented Oct 17, 2021

Uh oh!

uranusjr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 19, 2021

Uh oh!

kd2718 commented Oct 20, 2021

Uh oh!

Uh oh!

kd2718 commented Oct 20, 2021

Uh oh!

kd2718 commented Oct 25, 2021

Uh oh!

dimon222 commented Oct 25, 2021

Uh oh!

kd2718 commented Oct 29, 2021

Uh oh!

kd2718 commented Nov 1, 2021

Uh oh!

kd2718 commented Nov 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uranusjr Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

kd2718 Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

uranusjr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jedcunningham Nov 15, 2021

Choose a reason for hiding this comment

Uh oh!

kd2718 Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

kd2718 commented Nov 17, 2021

Uh oh!

kd2718 commented Nov 19, 2021

Uh oh!

uranusjr commented Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kd2718 commented Nov 29, 2021

Uh oh!

jedcunningham commented Nov 29, 2021

Uh oh!

boring-cyborg bot commented Nov 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

uranusjr commented Nov 19, 2021 •

edited

Loading