[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-16 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r750906779



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -658,11 +658,12 @@ def test_deserialize_model_file(self):
 def test_pod_name_confirm_to_max_length(self, _, pod_id):
 name = PodGenerator.make_unique_pod_id(pod_id)
 assert len(name) <= 253
-parts = name.split(".")
-if len(pod_id) <= 63:
+parts = name.split("-")
+if len(pod_id) <= 63 - 33:

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




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-16 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r750906688



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -658,11 +658,12 @@ def test_deserialize_model_file(self):
 def test_pod_name_confirm_to_max_length(self, _, pod_id):
 name = PodGenerator.make_unique_pod_id(pod_id)
 assert len(name) <= 253
-parts = name.split(".")
-if len(pod_id) <= 63:
+parts = name.split("-")
+if len(pod_id) <= 63 - 33:
 assert len(parts[0]) == len(pod_id)
 else:
 assert len(parts[0]) <= 63
+assert len(parts[1]) == 32
 assert len(parts[1]) <= 63

Review comment:
   good catch




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-16 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r750906589



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -658,11 +658,12 @@ def test_deserialize_model_file(self):
 def test_pod_name_confirm_to_max_length(self, _, pod_id):
 name = PodGenerator.make_unique_pod_id(pod_id)
 assert len(name) <= 253
-parts = name.split(".")
-if len(pod_id) <= 63:
+parts = name.split("-")
+if len(pod_id) <= 63 - 33:
 assert len(parts[0]) == len(pod_id)
 else:
 assert len(parts[0]) <= 63

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




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-11 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r747674223



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -684,7 +684,7 @@ def test_pod_name_is_valid(self, pod_id, 
expected_starts_with):
 len(name) <= 253 and all(ch.lower() == ch for ch in name) and 
re.match(regex, name)
 ), "pod_id is invalid - fails allowed regex check"
 
-assert name.rsplit(".")[0] == expected_starts_with
+assert name[:-33] == expected_starts_with

Review comment:
   `rsplit("-",1)[0]` works. Thanks for the tip. I am not as familiar with 
`rsplit`




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-10 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r747202513



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -684,7 +684,7 @@ def test_pod_name_is_valid(self, pod_id, 
expected_starts_with):
 len(name) <= 253 and all(ch.lower() == ch for ch in name) and 
re.match(regex, name)
 ), "pod_id is invalid - fails allowed regex check"
 
-assert name.rsplit(".")[0] == expected_starts_with
+assert name[:-33] == expected_starts_with

Review comment:
   ref: 
https://github.com/apache/airflow/pull/19036/files#diff-60bb2e2d34655edf905c00b495e1feee30aac430040bfca7995c812841c84916R689




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-10 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r747201088



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -684,7 +684,7 @@ def test_pod_name_is_valid(self, pod_id, 
expected_starts_with):
 len(name) <= 253 and all(ch.lower() == ch for ch in name) and 
re.match(regex, name)
 ), "pod_id is invalid - fails allowed regex check"
 
-assert name.rsplit(".")[0] == expected_starts_with
+assert name[:-33] == expected_starts_with

Review comment:
   Actually, this does create some issues. The test pod names are 
"pod-name-with-hyphen", so `name.rsplit("-")[0]` will just return `pod`. I 
switched it to use `"-".join(name.rsplit("-")[:-1])`. Unless we want to move 
back to the arbitrary `[:-33]` then this might be a better option.




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-10 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r747197691



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -684,7 +684,7 @@ def test_pod_name_is_valid(self, pod_id, 
expected_starts_with):
 len(name) <= 253 and all(ch.lower() == ch for ch in name) and 
re.match(regex, name)
 ), "pod_id is invalid - fails allowed regex check"
 
-assert name.rsplit(".")[0] == expected_starts_with
+assert name[:-33] == expected_starts_with

Review comment:
   My only worry is what if we change the "-" back to some other 
character... But then you could say the same worry about if we change the max 
pod length.




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-11-10 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r747197319



##
File path: tests/kubernetes/test_pod_generator.py
##
@@ -658,8 +658,8 @@ def test_deserialize_model_file(self):
 def test_pod_name_confirm_to_max_length(self, _, pod_id):
 name = PodGenerator.make_unique_pod_id(pod_id)
 assert len(name) <= 253
-parts = name.split(".")
-if len(pod_id) <= 63:
+parts = name.split("-")
+if len(pod_id) <= 63 - 33:
 assert len(parts[0]) == len(pod_id)
 else:
 assert len(parts[0]) <= 63

Review comment:
   This makes much more sense. thanks!




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-10-21 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r733802715



##
File path: airflow/kubernetes/pod_generator.py
##
@@ -459,10 +459,14 @@ def make_unique_pod_id(pod_id: str) -> str:
 return None
 
 safe_uuid = uuid.uuid4().hex  # safe uuid will always be less than 63 
chars
-# Strip trailing '-' and '.' as they can't be followed by '.'
-trimmed_pod_id = pod_id[:MAX_LABEL_LEN].rstrip('-.')
 
-safe_pod_id = f"{trimmed_pod_id}.{safe_uuid}"
+# Get prefix length after subtracting the uuid length. Clean up . and 
- from
+# end of podID because they can't be followed by '.'
+label_prefix_length = MAX_LABEL_LEN - len(safe_uuid) - 1 # -1 for 
separator

Review comment:
   need to add space 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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [airflow] kd2718 commented on a change in pull request #19036: replaced '.' with '-' and adjusted trimmed_pod_id per ticket comments

2021-10-18 Thread GitBox 


kd2718 commented on a change in pull request #19036:
URL: https://github.com/apache/airflow/pull/19036#discussion_r731487808



##
File path: airflow/kubernetes/pod_generator.py
##
@@ -460,9 +460,9 @@ def make_unique_pod_id(pod_id: str) -> str:
 
 safe_uuid = uuid.uuid4().hex  # safe uuid will always be less than 63 
chars
 # Strip trailing '-' and '.' as they can't be followed by '.'
-trimmed_pod_id = pod_id[:MAX_LABEL_LEN].rstrip('-.')
+trimmed_pod_id = pod_id[:MAX_LABEL_LEN-len(safe_uuid)].rstrip('-.')

Review comment:
   thanks. I have updated with these changes




-- 
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-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org