store: remove uses of docker namesgenerator#3354
Conversation
71a0ec3 to
358ec12
Compare
|
@crazy-max PTAL |
Asked Copilot about this list:
|
|
LOL fight of the AI - ChatGPT created the list. I'll probably split the list to 1 word per line; easier to add/remove things and to spot duplicates. Will update in a bit |
358ec12 to
87a05fb
Compare
|
Oh! Need to remove those words it flagged as possibly offensive. Funny because I told ChatGPT to take that into account but already noticed it didn't do a great job |
b4c3d6c to
dfbb95e
Compare
Yeah might not be the same model |
326e142 to
108ab3d
Compare
108ab3d to
0e64cb9
Compare
|
Oh! Let me check those last comments from CoPilot, LOL, I thought "let me put AI to use for silly tasks like generating a unique list", and it failed miserably every time. |
0e64cb9 to
1e5f5fa
Compare
|
Updated the list |
1e5f5fa to
b642e05
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the Docker namesgenerator dependency with a local implementation to avoid carrying debates around the choice of names (hackers/scientists) used in the Docker package. The change maintains the same random name generation functionality while using neutral adjectives, nouns, and themes.
- Removes external dependency on
github.com/docker/docker/pkg/namesgenerator - Implements local name generation using three word lists (adjectives, nouns, themes)
- Updates the random number generation to use
math/rand/v2
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| store/util.go | Updates name generation logic to use local word lists and new random API |
| store/random_names.go | Adds three word lists for generating random names with neutral vocabulary |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
b642e05
|
I think my understanding of where this issue landed was that @tonistiigi didn't want us to have a separate names generator or maintain separate code for buildx. I propose that either we close this PR or that we move this new names generator into the |
|
The "notable names" based generator will be sunset entirely, and won't be a separate module; moby/moby#52097 But I still wonder if buildx should use a different naming altogether for builders; see #3326 (comment)
|
b642e05 to
81c358a
Compare
|
@tonistiigi would you be ok if I resolved this by removing any random namesgenerator to begin with and change the naming to be |
It will not be part of the api or client modules, so replace it with a local implementation, that's not using names of hackers/scientists, so that we don't have to carry the debates around those choices here. Thanks to ChatGPT for generating some lists (I haven't fully checked them). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
81c358a to
745c571
Compare
|
hey @thaJeztah is this PR still being considered? You were right on this issue #3792 that the CVEs are false positives, but this namesgenerator is the only remaining case that imports from the old module, and it would make our security scanning software much happier to remove it 😅 if you could merge this soon that would be greatly appreciated! though i understand it's been a few months and there are merge conflicts for this work now edit: or alternatively the other PR mentioned here #3813 ? |
It will not be part of the api or client modules, so replace it with a local implementation, that's not using names of hackers/scientists, so that we don't have to carry the debates around those choices here.
Thanks to ChatGPT for generating some lists (I haven't fully checked them).