Generate custom container name on empty input#40432
Generate custom container name on empty input#40432
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes WSLC container creation so that when a caller does not provide a container name, the service generates a human-readable name (using a descriptor + mountain word list), and adds tests that validate the generated name format and uniqueness.
Changes:
- Add descriptor/mountain word lists for generated container names.
- Generate a unique container name during
WSLCSession::CreateContainer()when no name is provided. - Expand WSLC tests to validate the generated name format and uniqueness; update test include paths accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/windows/wslcsession/WSLCSession.cpp |
Adds container name generation helpers and injects generated names into container creation when name is absent. |
src/windows/wslcsession/ContainerNameGenerator.h |
Introduces the descriptor/mountain constants used for name generation and test validation. |
test/windows/WSLCTests.cpp |
Adds coverage for generated-name format and uniqueness when creating unnamed containers. |
test/windows/CMakeLists.txt |
Adds include path so tests can include the new name constants header. |
| // When retry > 0, appends a random digit (0-9) to reduce collisions. | ||
| std::string GenerateContainerName(int retry) | ||
| { | ||
| static std::mt19937 s_gen(std::random_device{}()); |
There was a problem hiding this comment.
I can't find a definite mention that this class is safe to use from multiple threads without synchronization.
Given that, I would recommend not marking it static unless we find a guarantee that it's thread safe
| std::uniform_int_distribution<size_t> leftDist(0, c_descriptors.size() - 1); | ||
| std::uniform_int_distribution<size_t> rightDist(0, c_mountains.size() - 1); | ||
|
|
||
| auto name = std::string(c_descriptors[leftDist(s_gen)]) + "_" + c_mountains[rightDist(s_gen)]; |
There was a problem hiding this comment.
nit: This could be simplified to:
auto name = std::format("{}_{}", c_descriptors[leftDist(s_gen)], c_mountains[rightDist(s_gen)]);
| std::unordered_set<std::string> existingNames; | ||
| for (const auto& c : m_containers) | ||
| { | ||
| existingNames.insert(c->Name()); |
There was a problem hiding this comment.
Looping through all the containers will make us create a copy of each container name. Instead of doing this, I think just iterating through m_containers after generating the name would be OK, since we expect conflicts to be very uniquely
| // Fallback to a GUID name | ||
| if (generatedName.empty()) | ||
| { | ||
| GUID guid{}; |
There was a problem hiding this comment.
Probably a good idea to log an event if we ever hit this
|
|
||
| // Generate a unique container name if the user didn't provide one. | ||
| std::string generatedName; | ||
| auto options = *containerOptions; |
There was a problem hiding this comment.
Copying the entire structure feels a bit rough here. Going through it, I think that the default copy behavior for this struct will do the right thing, but I would have a preference for adding an additional name argument to Create() so we don't have to do this
| add_library(wsltests SHARED ${SOURCES} ${HEADERS}) | ||
|
|
||
| target_include_directories(wsltests PRIVATE ${CMAKE_SOURCE_DIR}/src/windows/WslcSDK) | ||
| target_include_directories(wsltests PRIVATE ${CMAKE_SOURCE_DIR}/src/windows/WslcSDK ${CMAKE_SOURCE_DIR}/src/windows/wslcsession) |
There was a problem hiding this comment.
Instead of adding an include path, I would recommend moving that header to src/windows/common so we don't need to do a special case for it
Summary of the Pull Request
When containerOptions.Name is empty, generate our own container name.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed
Added tests.