Skip to content

move flightcontrol to use generics#3949

Merged
tonistiigi merged 1 commit intomoby:masterfrom
tonistiigi:flightcontrol-generics
Jun 29, 2023
Merged

move flightcontrol to use generics#3949
tonistiigi merged 1 commit intomoby:masterfrom
tonistiigi:flightcontrol-generics

Conversation

@tonistiigi
Copy link
Copy Markdown
Member

This should be much safer than the interface{} casts.

@tonistiigi tonistiigi requested a review from jedevc June 13, 2023 22:55
@tonistiigi
Copy link
Copy Markdown
Member Author

This exposed some "missing provenance" errors. I don't want to change that code again before #3945 is merged.

Comment on lines +180 to +182
if rp.provenance == nil {
return nil
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typed nil strikes again 😢

Copy link
Copy Markdown
Member

@jedevc jedevc Jun 21, 2023

Choose a reason for hiding this comment

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

Think it might be related to the change on line 280?

@jedevc
Copy link
Copy Markdown
Member

jedevc commented Jun 21, 2023

Once the provenance errors are sorted, this LGTM 🎉

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@tonistiigi tonistiigi force-pushed the flightcontrol-generics branch from e46296c to 8ffc03b Compare June 29, 2023 06:44
@tonistiigi
Copy link
Copy Markdown
Member Author

Green after other PRs merged as expected.

Comment thread solver/jobs.go
@@ -806,7 +808,7 @@ func (s *sharedOp) CacheMap(ctx context.Context, index int) (resp *cacheMapResp,
return nil, err
}
flightControlKey := fmt.Sprintf("cachemap-%d", index)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

non-blocking: we can remove this flightControlKey and just use index now, since we split up the flight control instances.

@tonistiigi tonistiigi merged commit ba96bfb into moby:master Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants