Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Exposing distro GUID in the registry #30

Merged
merged 13 commits into from
Jan 26, 2023
Merged

Exposing distro GUID in the registry #30

merged 13 commits into from
Jan 26, 2023

Conversation

EduardGomezEscandell
Copy link
Collaborator

@EduardGomezEscandell EduardGomezEscandell commented Jan 23, 2023 •
edited
Loading

This allows for ensuring continuity of a distro, i.e. ensuring that a distro with a certain name is the same as last time you checked.

WSL-316

@EduardGomezEscandell EduardGomezEscandell changed the title Expose distro Exposing distro ID in the registry Jan 23, 2023
@EduardGomezEscandell EduardGomezEscandell force-pushed the expose-distro-id branch 3 times, most recently from 4f66796 to 828954c Compare January 24, 2023 07:59
@EduardGomezEscandell EduardGomezEscandell changed the title Exposing distro ID in the registry Exposing distro GUID in the registry Jan 24, 2023
@EduardGomezEscandell EduardGomezEscandell mentioned this pull request Jan 24, 2023
Merged
@EduardGomezEscandell EduardGomezEscandell marked this pull request as ready for review January 25, 2023 07:47
@EduardGomezEscandell EduardGomezEscandell self-assigned this Jan 25, 2023
didrocks
didrocks requested changes Jan 25, 2023
View reviewed changes
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

And here we go! Some comments on some pieces to make the code itself more maintainable in the future and this PR a little bit cleaner, but those are just some nitpicks and general advice.

distro.go Outdated Show resolved Hide resolved
distro.go Outdated Show resolved Hide resolved
distro.go Outdated Show resolved Hide resolved
distro.go Show resolved Hide resolved
distro.go Show resolved Hide resolved
distro.go Show resolved Hide resolved
distro_test.go Show resolved Hide resolved
distro_test.go Outdated Show resolved Hide resolved
distro_test.go Outdated Show resolved Hide resolved
registry_windows.go Show resolved Hide resolved
@EduardGomezEscandell EduardGomezEscandell force-pushed the expose-distro-id branch 2 times, most recently from 36eaa18 to b0a3c14 Compare January 25, 2023 14:55
didrocks
didrocks requested changes Jan 25, 2023
View reviewed changes
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Some comments on the change (and probably some I missed in the previous reviews). Those are more areas to explore and then, we should be able to merge

distro.go Show resolved Hide resolved
distro_test.go Show resolved Hide resolved
distro_test.go Outdated Show resolved Hide resolved
distro_test.go Show resolved Hide resolved
distro_test.go Outdated Show resolved Hide resolved
@EduardGomezEscandell
Copy link
Collaborator Author

For some reason the fake distro GUID test sometimes fails because it doesn't error out. Working on it...

@EduardGomezEscandell
Moved registry functions to their own file
ef4119a
@EduardGomezEscandell
Implemented Distro.GUID
4816b6a
@EduardGomezEscandell
Added GUID to distro.String
1e9c703
@EduardGomezEscandell
Made GUID type alias private
379c7c5
@EduardGomezEscandell
Fixed distro leak
36d2949
@EduardGomezEscandell
GUID-related renaming
b49bdcf 
The word GUID is part of GoWSL's lexicon now, so it can be used
through the project
@EduardGomezEscandell
Expanded TestGUID
3b9e0ab 
- It now tests on wrong and non-existent distros
- It now tests that the GUID is actually initialized
@EduardGomezEscandell
Added comments to (guid|config)ToString and Distro.String
b8bd6f6
@EduardGomezEscandell
Decorated (*Distro)GUID()
de06a1c
@EduardGomezEscandell
Made (Distro).String less verbose for non-registered distros
0b8b449 
Commit b0a3c14 decorated the error
for (Distro).GUID which made it too verbose in this context in
particular.
@EduardGomezEscandell @didrocks
Typo in distro_test.go
01c8c75 
Co-authored-by: Didier Roche-Tolomelli <didier.roche@canonical.com>
@EduardGomezEscandell
Moved nolint directive to its own line
0edd741
@EduardGomezEscandell
registeredDistros: Fixed bug: was initializing slice, then appending
5874647 
Now I allocate, then append
@EduardGomezEscandell
Copy link
Collaborator Author

For some reason the fake distro GUID test sometimes fails because it doesn't error out. Working on it...

Fixed in #33. There was a collision between the names of realDistro and fakeDistro.

didrocks
didrocks approved these changes Jan 26, 2023
View reviewed changes
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

and let’s go! With the understanding of future test extensibility that is needed.

@EduardGomezEscandell EduardGomezEscandell merged commit 1d258d5 into main Jan 26, 2023
@EduardGomezEscandell EduardGomezEscandell deleted the expose-distro-id branch January 27, 2023 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@didrocks didrocks didrocks approved these changes

Assignees

@EduardGomezEscandell EduardGomezEscandell

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@EduardGomezEscandell @didrocks