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

Update crucible #2889

Merged
merged 6 commits into from
Apr 20, 2023
Merged

Update crucible #2889

merged 6 commits into from
Apr 20, 2023

Conversation

smklein
Copy link
Collaborator

@smklein smklein commented Apr 20, 2023 •
edited
Loading

I think this is blocking #1902 , because it's causing that PR to fail with a similar failure mode.

@smklein smklein requested a review from jmpesp April 20, 2023 16:21
leftwo
leftwo reviewed Apr 20, 2023
View reviewed changes
Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

Do we also need propolis to be updated as well?

@smklein
Copy link
Collaborator Author

smklein commented Apr 20, 2023

Do we? Has there been an API-breaking change in the past few months?

@leftwo
Copy link
Contributor

leftwo commented Apr 20, 2023

Do we? Has there been an API-breaking change in the past few months?

There have been a few changes to the message protocol between upstairs and downstairs (and
one that is currently in review) so depending on where propolis is, it could be incompatible with
latest. I'll go check and report back here.

@leftwo
Copy link
Contributor

leftwo commented Apr 20, 2023

So, we do need propolis to move forward.
Propolis is at: 894d44988c56713105bbbd07900d628781ab4dda

commit 894d44988c56713105bbbd07900d628781ab4dda
Author: liffy <[email protected]>
Date:   Wed Feb 8 11:18:54 2023 -0800

And, a change in message protocol happened here:

commit 75519cb2dc91be78ee5792718575e5348ddbdc1c
Author: Alan Hanson <[email protected]>
Date:   Mon Feb 27 05:50:15 2023 -0800

So, at a minimum propolis will need that.

leftwo
leftwo reviewed Apr 20, 2023
View reviewed changes
Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

When Gregs update to propolis lands, we will want that to be included here as well.

@smklein smklein merged commit bd66f33 into main Apr 20, 2023
@smklein smklein deleted the update-crucible branch April 20, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leftwo leftwo leftwo left review comments

@jmpesp jmpesp jmpesp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@smklein @leftwo @jmpesp