-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
Added state upgrade
#3467
Added state upgrade
#3467
Conversation
@@ -54,14 +54,13 @@ func TestArtifact_Dependencies(t *testing.T) { | |||
want: []string{ | |||
"00000000-0000-0000-0000-000000000002", | |||
"00000000-0000-0000-0000-000000000003", | |||
"00000000-0000-0000-0000-000000000001", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old assertion failed due to my fix here:
Before that you would get artifacts that had technically already been "seen". ie. asking for deps on artifact ID 1
would return 1
. Which is not expected (and ironically; also a cycle).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great to me, just some minor comments and one question.
I would also recommend running the cve
integration tests as well as any that depend on change summaries just to ensure that the changes there didn't affect anything.
other: "[CYAN]{{.V0}}[/RESET] > [BOLD][ACTIONABLE]{{.V1}}[/RESET]" | ||
name: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this not need a second [/RESET]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the resets act on everything. It's basically telling the shell "go back to normal rendering".
internal/runners/upgrade/upgrade.go
Outdated
bumpedBS.SetAtTime(ts) | ||
|
||
if bumpedBS.AtTime().String() == localCommit.BuildScript().AtTime().String() { | ||
panic(fmt.Sprintf("bumped buildscript is same as local commit, old: %s, new: %s", bumpedBS.AtTime(), localCommit.BuildScript().AtTime())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this leftover from initial development? If not I'm wondering why we would add a panic for a known error case that we can handle in a more graceful manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woops, it is indeed leftover. Thanks for spotting!
@@ -112,3 +114,23 @@ func ToLookupMapByKey[T any, K string | int | strfmt.UUID](data []T, keyCb func( | |||
} | |||
return result | |||
} | |||
|
|||
// EqualValues checks if two slices have equal values, regardless of ordering. This does not recurse into nested slices or structs. | |||
func EqualValues[S ~[]E, E cmp.Ordered](a, b S) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Unit test for this function would be nice.
Couple of notable supplemental changes to facilitate
state upgrade
:buildplan.Diff()
) to facilitate iteration of all changes regardless of type.--ts
values moved to runbit.buildplan.*.Dependencies()
now take an extra argument that allows you to skip recursion for certain artifacts.sliceutils.EqualValues
which lets you compare two slices regardless of their ordering.output.Structured()
which allows you to print structured data without needing to create your own marshaller.