-
Notifications
You must be signed in to change notification settings - Fork 751
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
Sensor name convention hashed #1597
base: master
Are you sure you want to change the base?
Sensor name convention hashed #1597
Conversation
… as label to sensor object for consistency in querying
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All in all great stuff. I would hold off on this for just a bit until #1608 moves forward so we know how to proceed with regards to sensors.
A big question with both the general argo naming convention changes, and this PR is, how to cleanly handle existing deployments so that no resources are left dangling when redeploying with a new naming schema.
if self._sensor: | ||
# -----Added---- | ||
# Set the original workflow name as a label on the sensor | ||
self._sensor.labels = {self.name} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1608 is aiming to fix this, but in general, the full workflow name can be quite lengthy, so I would suggest recording it in annotations instead in order to not hit any length limits.
@@ -82,8 +82,18 @@ def list_executions(self, state_machine_arn, states): | |||
) | |||
|
|||
def terminate_execution(self, state_machine_arn, execution_arn): | |||
# TODO | |||
pass | |||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated changes to this PR
hashed_name = hashlib.sha256(self.name.encode()).hexdigest()[ | ||
:10 | ||
] # Get first 10 chars of the hash | ||
sensor_name = f"s-{hashed_name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does argo append the random suffix in all sensor deployments, or only in cases where the length exceeds 63 characters? what we usually aim to do is retain the original name, but if truncation needs to happen, chop it up and append a hash of the value as a suffix.
This way at at least part of the name remains descriptive
Made a new implementation for the naming convention of the sensor objects with a hash, to ensure all instances fit within th 63 character limit as issue #1591 outlined that metaflow executed normal operations despite the object not being deployed.
Label added to sensor object to maintain data integrity of the original workflow name.