Skip to content

Conversation

maxhniebergall
Copy link
Contributor

@maxhniebergall maxhniebergall commented Dec 21, 2023

This change adds requirements for inference service model IDs. Specifically, Model IDs must can contain only lowercase alphanumeric (a-z and 0-9), hyphens or underscore characters and must start and end with lowercase alphanumeric.

closes https://github.com/elastic/ml-team/issues/1093

@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.13.0 labels Dec 21, 2023
@maxhniebergall maxhniebergall added :ml Machine learning >non-issue and removed needs:triage Requires assignment of a team area label labels Dec 21, 2023
@elasticsearchmachine elasticsearchmachine added the Team:ML Meta label for the ML team label Dec 21, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you change the PR description to a standalone explanation of what's been done. Linking to a private issue is no use for a user who reads the release note, navigates from it to this PR, and tries to understand what it's doing.

They'll find it more helpful if you say that a valid ID is a string that contains lower case characters, digits, hyphens, underscores or dots, and must start and end only in lower case characters or digits.

@@ -33,6 +33,38 @@ public final class MlStrings {

public static final int ID_LENGTH_LIMIT = 64;

// for testing
public static String[] someInvalidChars = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this final:

    public static final String[] SOME_INVALID_CHARS = {

Also, please move it to MlStringsTests, as it's purely for testing and doesn't need to be built into the production jars.

@elasticsearchmachine
Copy link
Collaborator

Hi @maxhniebergall, I've created a changelog YAML for you.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if you could just rename the variable.

maxhniebergall and others added 2 commits December 22, 2023 11:00
Co-authored-by: David Roberts <dave.roberts@elastic.co>
@maxhniebergall maxhniebergall merged commit 3a2d9be into main Dec 22, 2023
@maxhniebergall maxhniebergall deleted the validateInferenceModelIds branch December 22, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :ml Machine learning Team:ML Meta label for the ML team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants