SS-1398 mlflow subdomain bug fix #316
Conversation
apps/types_/subdomain.py
Outdated
| regex=r"^(?!-)(?!.*mlflow)[a-z0-9-]{3,53}(?<!-)$", | ||
| message="Subdomain must be 3-53 characters long, contain only lowercase letters, digits, hyphens, " | ||
| "and cannot start or end with a hyphen", | ||
| "cannot contain 'mlflow', " |
There was a problem hiding this comment.
I personally don't like that it'll be shown for all apps, but we can discuss this
And could you please update the test case for this?
There was a problem hiding this comment.
Thank you. I used a different approach and fixed it from the secret name. The root cause of the mlflow subdomain bug is that, the chart avoids redundant naming when the release name already contains mlflow, but appends -mlflow otherwise. This conditional logic is why only mlflow-containing subdomains fail. The logic is implemented here: . From there,
If release name contains chart name it will be used as a full name.
I checked and it fixed the bug.
| # If release name contains chart name it will be used as a full name. | ||
| # see here: https://github.com/bitnami/charts/blob/main/bitnami/common/templates/_names.tpl#L21-L37 | ||
| if "mlflow" in subdomain.subdomain.lower(): | ||
| secret_name = f"{subdomain.subdomain}-tracking" | ||
| else: | ||
| secret_name = f"{subdomain.subdomain}-mlflow-tracking" |
There was a problem hiding this comment.
Nice work, thank you!
I think this will be more intuitive:)
There was a problem hiding this comment.
Thank you! Yes, I agree :)
churnikov
left a comment
churnikov
left a comment
There was a problem hiding this comment.
One thing though, could you please update the manual test case for mlflow to reflect this issue?
I will work on it. |
hamzaimran08
left a comment
hamzaimran08
left a comment
There was a problem hiding this comment.
Looks good to me. Only thing I would point out is there seems to be a light blue color on the Copy button after you click it (for me atleast). If that could be changed to one of the Serve colors that would be great. Good work.
Status
Ready for review
Description
Source: https://scilifelab.atlassian.net/browse/SS-1398
mlflowin it, secrets are not generated according to thesecrets pagepatternSource: https://scilifelab.atlassian.net/browse/SS-1397
The root cause of the mlflow subdomain bug is that, the chart avoids redundant naming when the release name already contains mlflow, but appends -mlflow otherwise. This conditional logic is why only mlflow-containing subdomains fail. The logic is implemented here: . From there,
If release name contains chart name it will be used as a full name.Types of changes
bugfix
Checklist
If you're unsure about any of the items below, don't hesitate to ask. We're here to help!
This is simply a reminder of what we are going to look for before merging your code.
Further comments
Anything else you think we should know before merging your code!