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.

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

Instance resize #2487

Merged
merged 18 commits into from
Nov 27, 2024
Merged

Instance resize #2487

merged 18 commits into from
Nov 27, 2024

Conversation

david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented Oct 4, 2024

Based on oxidecomputer/omicron#6774. Wait for that to merge so we can point at omicron main.

Copy link

vercel bot commented Oct 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Nov 26, 2024 7:44pm

@david-crespo david-crespo marked this pull request as ready for review October 4, 2024 18:37
@david-crespo david-crespo changed the title Don't be broken by instance resize API change Instance resize Oct 4, 2024
* Switch to regular modal

* Improve modal close button alignment

* Semi 'currently using:' and fix semi weight

* Revert: fix semi weight

Handled in #2496 instead

* Switch route for modal

* Disable submit if the specs are the same

* Improve toast

* Fix lint error `no-unused-expressions`
@david-crespo
Copy link
Collaborator Author

I did something gnarly, but luckily it seems to have worked. I pushed my own version of the omicron branch with omicron main merged in to get everything else up to date, then I bumped the API here with that commit, then I bumped omicron to main on console main, than I merged console main into here.

Short version: this is pretty up to date and once resize is merged in the API we will be in good shape. This still needs a bit of polish, though.

@charliepark
Copy link
Contributor

No rush at all; just wanted to note that oxidecomputer/omicron#6774 has been merged.

Copy link
Contributor

@charliepark charliepark left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@david-crespo
Copy link
Collaborator Author

I think we could get the info box on one line. Don’t really need the instance name. Maybe just Current: 8 CPU / 4 GiB (whatever the canonical unit for CPU is)

IMG_7948

@benjaminleonard
Copy link
Contributor

I added that in there because you can resize from the index, at which point it might not be clear which instance you are resizing. Or at least it's helpful to reassure the user of their action.

@charliepark
Copy link
Contributor

charliepark commented Nov 25, 2024

Screenshot 2024-11-25 at 2 23 41 PM

@charliepark
Copy link
Contributor

It seemed useful to me to have the name in the box, unless there's some other way to verify which instance they're updating. I thought putting it in the modal header (Resize instance db1) could work, but that's not a pattern we have in other places in the app.

@david-crespo
Copy link
Collaborator Author

david-crespo commented Nov 26, 2024

Oh right, I forgot about resizing from the list view.

Overflow with a long name is kinda barf, maybe I was wrong about one line. The modal just feels so stubby.

image

Putting the name in the title is kind of cool but it might have the overflow problem too.

image

@benjaminleonard
Copy link
Contributor

Fixed the truncation. Design-wise looks good to me.

@charliepark
Copy link
Contributor

🎉

@charliepark charliepark merged commit 059c551 into main Nov 27, 2024
8 checks passed
@charliepark charliepark deleted the instance-resize branch November 27, 2024 15:03
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.

3 participants