Skip to content

InABox: Check snapshot regions before creating box#222

Merged
Davis-A merged 2 commits intofastmail:mainfrom
Davis-A:InABox-check-snapshot-regions
Feb 13, 2024
Merged

InABox: Check snapshot regions before creating box#222
Davis-A merged 2 commits intofastmail:mainfrom
Davis-A:InABox-check-snapshot-regions

Conversation

@Davis-A
Copy link
Copy Markdown
Member

@Davis-A Davis-A commented Feb 9, 2024

When a snapshot is unavailable in the users datacentre it's much nicer to reply with a better response than "Something weird happened..."

@Davis-A Davis-A self-assigned this Feb 9, 2024
@Davis-A Davis-A changed the title InABox: Check snapshot regions before creating box WIP: InABox: Check snapshot regions before creating box Feb 9, 2024
@Davis-A
Copy link
Copy Markdown
Member Author

Davis-A commented Feb 9, 2024

The functionality is there. But i need to update the test suite before merging.

@Davis-A Davis-A force-pushed the InABox-check-snapshot-regions branch from cc805ba to 1f90f99 Compare February 9, 2024 02:20
@Davis-A Davis-A changed the title WIP: InABox: Check snapshot regions before creating box InABox: Check snapshot regions before creating box Feb 9, 2024
@Davis-A
Copy link
Copy Markdown
Member Author

Davis-A commented Feb 9, 2024

Tests fixed so i think this is good to go

@Davis-A Davis-A requested a review from rjbs February 9, 2024 02:28
Comment thread lib/Synergy/Reactor/InABox.pm Outdated

if ($compatible_regions) {
return await $event->reply(
"Unable to create snapshot in region '$region. Available compatible regions are $compatible_regions."
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: missing closing quote after '$region

Same below

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.

(yup)

@wolfsage
Copy link
Copy Markdown
Contributor

wolfsage commented Feb 9, 2024

Nice, though you aren't testing for the new error conditions? Would be nice to!

Copy link
Copy Markdown
Member

@rjbs rjbs left a comment

Choose a reason for hiding this comment

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

You're going to have to change this when you rebase after the big test improvements, but it should be easy.

But also, I thing you've got more API requests than are needed for this? And testing the failure mode is probably a good idea, as Matthew said.

Comment thread lib/Synergy/Reactor/InABox.pm Outdated

# check snapshot is available in the users region
my %snapshot_regions = map {; $_ => 1 }
await $self->_get_snapshot_regions($snapshot->{id});
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.

This seems off to me. Above, with this line…

my $snapshot = await $self->_get_snapshot($version);

…it seems like we should have a snapshot already retrieved. Then, _get_snapshot_regions goes and fetches it again. Meanwhile, the API reference suggests that the objects returned from _get_snapshot should already have the region data. That would mean that this branch is adding another round trip and another place for things to fail.

Can you, instead, look at $snapshot->{regions} without making another request?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am indeed an idiot. For some reason my brain decided that $snapshot was just an id, but clearly it's a hash because i'm getting the id from it 🤦

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.

If this is the worst mistake you make all week, you'll be doing great. 😄

When a snapshot is unavailable in the users datacentre it's much nicer
to reply with a better response than "Something weird happened..."
@Davis-A Davis-A force-pushed the InABox-check-snapshot-regions branch 3 times, most recently from d964dd3 to 290ffff Compare February 13, 2024 00:09
This adds the required data to the existing tests,
And adds two tests for checking the snapshot region
@Davis-A Davis-A force-pushed the InABox-check-snapshot-regions branch from 290ffff to dc6c6c2 Compare February 13, 2024 00:11
@Davis-A
Copy link
Copy Markdown
Member Author

Davis-A commented Feb 13, 2024

Okie dokie. Re-done without my 🤦 error.

Also added tests for the snapshot check too.

adavis@carbon:~/src/Synergy$ yath t/inabox.t

** Defaulting to the 'test' command **

( PASSED )  job  1    t/inabox.t

                                Yath Result Summary
-----------------------------------------------------------------------------------
     File Count: 1
Assertion Count: 33
      Wall Time: 3.28 seconds
       CPU Time: 3.69 seconds (usr: 0.12s | sys: 0.04s | cusr: 2.99s | csys: 0.54s)
      CPU Usage: 112%
    -->  Result: PASSED  <--

@Davis-A Davis-A requested a review from rjbs February 13, 2024 00:12
Copy link
Copy Markdown
Member

@rjbs rjbs left a comment

Choose a reason for hiding this comment

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

Love it.

@Davis-A Davis-A merged commit 9cd474e into fastmail:main Feb 13, 2024
@Davis-A Davis-A deleted the InABox-check-snapshot-regions branch February 13, 2024 00:42
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