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

[GitBox <gi...@apache.org>]

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



##########
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 '.'

Review comment:
       ```suggestion
           # Get prefix length after subtracting the uuid length. Clean up '.' and '-' from
           # end of podID ('.' can't be followed by '-').
   ```
   
   Technically we no longer need to remove any trailing`-`, but I think it makes sense to continue doing so.

##########
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:
       ```suggestion
           parts = name.split("-")
           assert parts[0] == pod_id[:30]
   ```
   
   Also the following line I can't suggest, line 666, no longer makes sense imo. We should check it against a fixed length, I think this is right:
   
   ```
   assert len(parts[1]) == 32
   ```

##########
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:
       ```suggestion
           assert name.rsplit("-")[0] == expected_starts_with
   ```
   
   Nit, I think the `rsplit` approach is easier to grok?

##########
File path: tests/kubernetes/test_pod_generator.py
##########
@@ -497,7 +497,7 @@ def test_ensure_max_label_length(self, mock_uuid):
             base_worker_pod=worker_config,
         )
 
-        assert result.metadata.name == 'a' * 63 + '.' + self.static_uuid.hex
+        assert result.metadata.name == ('a' * 63)[:-33] + '-' + self.static_uuid.hex

Review comment:
       ```suggestion
           assert result.metadata.name == 'a' * 30 + '-' + self.static_uuid.hex
   ```




-- 
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