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

Use comma-delineated list for OS in image list generation #32291

Merged
merged 1 commit into from
Apr 23, 2021
Merged

Use comma-delineated list for OS in image list generation #32291

merged 1 commit into from
Apr 23, 2021

Conversation

nickgerace
Copy link
Contributor

@nickgerace nickgerace commented Apr 22, 2021 •
edited
Loading

Relevant issue: #32290
Original solution basis: https://play.golang.org/p/_Dj725132Lm
Refactor basis: https://play.golang.org/p/k-6IyqTPE66
Solution Explanation: #32291 (comment)

@nickgerace nickgerace self-assigned this Apr 22, 2021
@nickgerace nickgerace changed the title Use comma-delineated list for OS in image list generation [WIP] Use comma-delineated list for OS in image list generation Apr 22, 2021
@nickgerace nickgerace force-pushed the master branch 4 times, most recently from f4e9021 to 289a966 Compare April 22, 2021 19:00
@nickgerace nickgerace mentioned this pull request Apr 22, 2021
Closed
@nickgerace
Copy link
Contributor Author

Solution Explanation: The ideal solution would be to remove the concept of OSType since the existing code creates rancher-images.txt and then rancher-windows-images.txt (O(n^2)). However, this solution aims to add the comma-delineated list functionality to the existing structure, rather than uprooting it. Moreover, there are only two OSType constants, so the runtime complexity is minimally impacted (realistically around O(2n)).

@nickgerace nickgerace added this to the v2.5.8 milestone Apr 22, 2021
@nickgerace nickgerace changed the title [WIP] Use comma-delineated list for OS in image list generation Use comma-delineated list for OS in image list generation Apr 22, 2021
thedadams
thedadams suggested changes Apr 22, 2021
View reviewed changes
pkg/image/resolve.go Outdated Show resolved Hide resolved
pkg/image/resolve.go Outdated Show resolved Hide resolved
pkg/image/resolve.go Outdated Show resolved Hide resolved
pkg/image/resolve.go Outdated Show resolved Hide resolved
thedadams
thedadams previously approved these changes Apr 22, 2021
View reviewed changes
@nickgerace nickgerace requested a review from cbron April 22, 2021 20:33
@nickgerace
Copy link
Contributor Author

Rebased after Donnie's approval

aiyengar2
aiyengar2 approved these changes Apr 22, 2021
View reviewed changes
Copy link
Contributor

@aiyengar2 aiyengar2 left a comment

Choose a reason for hiding this comment

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

LGTM

aiyengar2
aiyengar2 suggested changes Apr 22, 2021
View reviewed changes
pkg/image/resolve.go Show resolved Hide resolved
This was referenced Apr 22, 2021
Merged
Merged
cbron
cbron suggested changes Apr 23, 2021
View reviewed changes
pkg/image/resolve.go Outdated Show resolved Hide resolved
pkg/image/resolve.go Outdated Show resolved Hide resolved
aiyengar2
aiyengar2 previously approved these changes Apr 23, 2021
View reviewed changes
@nickgerace nickgerace force-pushed the master branch 2 times, most recently from 3f1f87b to ea39770 Compare April 23, 2021 14:03
aiyengar2
aiyengar2 previously approved these changes Apr 23, 2021
View reviewed changes
@nickgerace nickgerace requested a review from cbron April 23, 2021 16:45
@nickgerace nickgerace force-pushed the master branch 3 times, most recently from bd063b9 to bdca878 Compare April 23, 2021 22:25
@nickgerace nickgerace requested a review from cbron April 23, 2021 23:08
@nickgerace nickgerace merged commit 7aec8f7 into rancher:master Apr 23, 2021
@nickgerace nickgerace mentioned this pull request Apr 23, 2021
Merged
@shpwrck shpwrck added the kind/enhancement Issues that improve or augment existing functionality label May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aiyengar2 aiyengar2 aiyengar2 left review comments

@cbron cbron cbron approved these changes

@thedadams thedadams thedadams left review comments

Assignees

@nickgerace nickgerace

Labels
kind/enhancement Issues that improve or augment existing functionality
Projects
None yet
Milestone
v2.5.8
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nickgerace @cbron @thedadams @aiyengar2 @shpwrck