Derive profile name from display name in `ProfileService.CreateProfile` #4335

gajananan · 2024-09-01T07:45:16Z

Summary

Currently, clients of the API are required to derive the profile name from the display name using various transformations. This results in duplicated logic across the ecosystem, increasing complexity and potential inconsistencies.

This commit scopes the logic to derive the profile name from the display name to the ProfileService.CreateProfile endpoint. By centralizing this logic, we reduce redundancy and simplify client code. Clients of the API are no longer need to implement logic to deriive the profile name when creating a new profile.

Fixes #2943

Change Type

Mark the type of change your PR introduces:

Bug fix (resolves an issue without affecting existing features)
Feature (adds new functionality without breaking changes)
Breaking change (may impact existing functionalities or require documentation updates)
Documentation (updates or additions to documentation)
Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

Reviewed my own code for quality and clarity.
Added comments to complex or tricky code sections.
Updated any affected documentation.
Included tests that validate the fix or feature.
Checked that related changes are merged.

Currently, clients of the API are required to derive the profile name from the display name using various transformations. This results in duplicated logic across the ecosystem, increasing complexity and potential inconsistencies. This commit scopes the logic to derive the profile name from the display name to the ProfileService.CreateProfile endpoint. By centralizing this logic, we reduce redundancy and simplify client code. Clients of the API are no longer need to implement logic to deriive the profile name when creating a new profile. Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

jhrozek · 2024-09-02T19:56:25Z

internal/profiles/util.go

+}
+
+// The profile name should be derived from the profile display name given the following logic
+func CleanDisplayName(displayName string) string {


this function probably doesn't have to be exported

Thank you, @jhrozek. Fixed now.

jhrozek · 2024-09-02T19:57:14Z

internal/profiles/util.go

+	displayName = strings.TrimSpace(displayName)
+
+	// Remove non-alphanumeric characters
+	re := regexp.MustCompile(`[^a-zA-Z0-9\s]`)


if the regexes are not going to change, then it could probably be a global but non exported variable

Thank you, @jhrozek. Fixed now

jhrozek

Thank you for your contribution @gajananan !

I'm going to tag @blkt who was recently discussing the semantics of display names and might have better opinions on that than I. In the meantime I just put some general comments into the PR and I'm going to let the CI pipeline run.

eleftherias

This is looking great so far @gajananan.
I know you had some questions about his requirement:

If the derived profile name already exists in the same project, then we append a -1 to the end of the derived profile name (my_profile-1).
If the new name also exists in the project, then we increment the appending digit by 1 until we find a name that doesn't already exist. (my_profile-2, my_profile-3 etc)

To fulfil this, you will have to add an extra call to fetch all the profiles from the the current project, and then pass their names to DeriveProfileNameFromDisplayName.

Once the "clean" profile name is derived, you'll need to check it against all the existing names.

Some example inputs and outputs:
DeriveProfileNameFromDisplayName("My profile", ["other_profile", "custom_profile") -> "my_profile"

DeriveProfileNameFromDisplayName("My profile", ["other_profile", "my_profile") -> "my_profile-1"

DeriveProfileNameFromDisplayName("My profile", ["other_profile", "my_profile", "my_profile-1") -> "my_profile-2"

Feel free to reach out if this is unclear or if you have any questions.

internal/profiles/util.go

Define regexes to be a global but non exported variable. Refactor unit test to test `DeriveProfileNameFromDisplayName` instead of `cleanDisplayName` which is not exported anymore. Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

Handle cases where the derived profile name already exists in the same project. Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

Remove empty line Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

gajananan · 2024-09-08T09:52:28Z

This is looking great so far @gajananan. I know you had some questions about his requirement:

If the derived profile name already exists in the same project, then we append a -1 to the end of the derived profile name (my_profile-1).
If the new name also exists in the project, then we increment the appending digit by 1 until we find a name that doesn't already exist. (my_profile-2, my_profile-3 etc)

To fulfil this, you will have to add an extra call to fetch all the profiles from the the current project, and then pass their names to DeriveProfileNameFromDisplayName.

Once the "clean" profile name is derived, you'll need to check it against all the existing names.

Some example inputs and outputs: DeriveProfileNameFromDisplayName("My profile", ["other_profile", "custom_profile") -> "my_profile"

DeriveProfileNameFromDisplayName("My profile", ["other_profile", "my_profile") -> "my_profile-1"

DeriveProfileNameFromDisplayName("My profile", ["other_profile", "my_profile", "my_profile-1") -> "my_profile-2"

Feel free to reach out if this is unclear or if you have any questions.

I have updated the PR to implement this. Please let me know if the approach needs to change.

coveralls · 2024-09-09T09:16:07Z

coverage: 53.424% (-0.5%) from 53.919%
when pulling c519556 on gajananan:drive-name-from-displayname
into b1f13f9 on stacklok:main.

Added additional test to increase coverage Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

eleftherias · 2024-09-09T14:45:59Z

internal/profiles/util_test.go

+			expected:             "my_profile-1",
+		},
+		{
+			name: "Derived profile name does exist in the current project",


There are 2 tests with the same name "Derived profile name does exist in the current project", which will make debugging difficult if one fails.

eleftherias

Thanks for the updates @gajananan! I've left a couple of comments inline.

eleftherias · 2024-09-09T14:51:34Z

internal/profiles/util.go

+
+	// check if the current project already has a profile with that name, then add a counter
+	for strings.Contains(strings.Join(existingProfileNames, " "), derivedName) {
+		derivedName = fmt.Sprintf("%s-%d", name, counter)


There are a few edge cases here where the name will exceed 63 characters once we append the counter.
If the counter is a single digit, then we need to remove 2 character from the name, if it's longer than 61 characters.
As the counter grows, it means we'll have to remove more characters from the name, for example, it we append -123 then we need to remove 4 characters from the name, if it's longer than 61 characters.

Thank you, I fixed the logic to handle the edge cases mentioned as well as added unit tests for them. @eleftherias

Fixed duplicated test names Fixed DeriveProfileNameFromDisplayName to handle the edge cases where the name will exceed 63 characters once we append the counter. Added unit-tests for edge cases Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

Remoed hardcoded value for profileNameMaxLength Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

eleftherias · 2024-09-13T12:03:37Z

Thank you for you contribution @gajananan!

gajananan · 2024-09-13T21:06:16Z

Thank you for you contribution @gajananan!

Thank you @eleftherias for clarifying the requirements and the follow up feedback.

gajananan force-pushed the drive-name-from-displayname branch from 2b95134 to e0f79e2 Compare September 1, 2024 08:12

gajananan force-pushed the drive-name-from-displayname branch from e0f79e2 to 7b54130 Compare September 1, 2024 08:35

jhrozek reviewed Sep 2, 2024

View reviewed changes

eleftherias self-requested a review September 3, 2024 09:42

eleftherias reviewed Sep 3, 2024

View reviewed changes

internal/profiles/util.go Outdated Show resolved Hide resolved

gajananan added 3 commits September 7, 2024 21:20

Derive profile name from display name in ProfileService.CreateProfile

0fb4539

Handle cases where the derived profile name already exists in the same project. Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

Derive profile name from display name in ProfileService.CreateProfile

fd597b9

Remove empty line Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

gajananan marked this pull request as ready for review September 8, 2024 12:13

Derive profile name from display name in ProfileService.CreateProfile

2aaba5f

Added additional test to increase coverage Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

eleftherias reviewed Sep 9, 2024

View reviewed changes

gajananan added 2 commits September 13, 2024 19:50

Derive profile name from display name in ProfileService.CreateProfile

c519556

Remoed hardcoded value for profileNameMaxLength Signed-off-by: Kugamoorthy Gajananan <gajananan@gmail.com>

eleftherias approved these changes Sep 13, 2024

View reviewed changes

eleftherias merged commit 6cf6503 into stacklok:main Sep 13, 2024
19 of 21 checks passed

BrewTestBot mentioned this pull request Sep 17, 2024

minder 0.0.63 Homebrew/homebrew-core#190990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Derive profile name from display name in `ProfileService.CreateProfile` #4335

Derive profile name from display name in `ProfileService.CreateProfile` #4335

gajananan commented Sep 1, 2024 •

edited

Loading

jhrozek Sep 2, 2024

gajananan Sep 7, 2024

jhrozek Sep 2, 2024

gajananan Sep 7, 2024

jhrozek left a comment

eleftherias left a comment

gajananan commented Sep 8, 2024

coveralls commented Sep 9, 2024 •

edited

Loading

eleftherias Sep 9, 2024

eleftherias left a comment

eleftherias Sep 9, 2024

gajananan Sep 13, 2024

eleftherias commented Sep 13, 2024

gajananan commented Sep 13, 2024

Derive profile name from display name in ProfileService.CreateProfile #4335

Derive profile name from display name in ProfileService.CreateProfile #4335

Conversation

gajananan commented Sep 1, 2024 • edited Loading

Summary

Change Type

Testing

Review Checklist:

jhrozek Sep 2, 2024

Choose a reason for hiding this comment

gajananan Sep 7, 2024

Choose a reason for hiding this comment

jhrozek Sep 2, 2024

Choose a reason for hiding this comment

gajananan Sep 7, 2024

Choose a reason for hiding this comment

jhrozek left a comment

Choose a reason for hiding this comment

eleftherias left a comment

Choose a reason for hiding this comment

gajananan commented Sep 8, 2024

coveralls commented Sep 9, 2024 • edited Loading

eleftherias Sep 9, 2024

Choose a reason for hiding this comment

eleftherias left a comment

Choose a reason for hiding this comment

eleftherias Sep 9, 2024

Choose a reason for hiding this comment

gajananan Sep 13, 2024

Choose a reason for hiding this comment

eleftherias commented Sep 13, 2024

gajananan commented Sep 13, 2024

Derive profile name from display name in `ProfileService.CreateProfile` #4335

Derive profile name from display name in `ProfileService.CreateProfile` #4335

gajananan commented Sep 1, 2024 •

edited

Loading

coveralls commented Sep 9, 2024 •

edited

Loading