Um. You just broke the custom storage tests, because select_disks() uses DISK_CUSTOM to decide whether to click the custom storage checkbox.

Please at least grep through the code for any variable before removing it!

If you want to keep this main.pm simplification I guess we could set up $partitioning like this:

# Set $partitioning to DISK_GUIDED or DISK_CUSTOM if either is set, otherwise ''
my $partitioning = $DISK_GUIDED ? $DISK_GUIDED : $DISK_CUSTOM ? $DISK_CUSTOM : '';

I know, I know, it's perl-y, but easy to comment. Make sure to set it in the right scope for both code blocks to see it, then they can both use it.

Well, it winds up being:

my $partitioning = get_var("DISK_GUIDED") ? 'guided_'.get_var("DISK_GUIDED") : get_var("DISK_CUSTOM") ? 'custom_'.get_var("DISK_CUSTOM") : '';

which works, but looks a bit icky. I dunno, let me know what you think. edit: hah, it's so long phabricator chokes on it...

Now I see that in an un-Phab'ed follow-up commit, jsedlak changed it so that the sole current custom test clicks the custom partitioning box itself.

I don't like that, though, because it means we have to write that into *every single custom partitioning test we ever make*, and if the route to custom partitioning ever changes we get to change all of those tests. I preferred the way I had it, where select_disks() did it.

We could put it back that way just by having select_disks() check if PARTITIONING starts with custom_.

In D486#9169, @adamwill wrote:

Now I see that in an un-Phab'ed follow-up commit, jsedlak changed it so that the sole current custom test clicks the custom partitioning box itself.

I don't like that, though, because it means we have to write that into *every single custom partitioning test we ever make*, and if the route to custom partitioning ever changes we get to change all of those tests. I preferred the way I had it, where select_disks() did it.

We could put it back that way just by having select_disks() check if PARTITIONING starts with custom_.

Yep, I have fixed it so that tests will run properly, I've discussed it with jskladan and hotpatched it. I mean, we could open up a ticket and discuss it here. I should have waited a little longer, I'm sorry. I saw that DISK_CUSTOM and DISK_GUIDED are doing basically the same (meaning they load whatever test you specify) and they were mutually exclusive so there was no point in keeping them both (I have encountered it during documenting what variable does what, that there is no difference between what you should put into DISK_GUIDED and what you should put into DISK_CUSTOM).

I did actually consider that while writing the main.pm changes, but in the end I thought we already have one use for distinguishing between the two (the one that got broken) and it seemed plausible that in future we'd have other reasons that maybe we'd want to be able to tell the difference easily.

Still, we can keep it this way and just figure out the nicest perl way to do if get_var('PARTITIONING').startswith('custom_'), I guess. No biggy.

In D486#9171, @adamwill wrote:

I did actually consider that while writing the main.pm changes, but in the end I thought we already have one use for distinguishing between the two (the one that got broken) and it seemed plausible that in future we'd have other reasons that maybe we'd want to be able to tell the difference easily.

Still, we can keep it this way and just figure out the nicest perl way to do if get_var('PARTITIONING').startswith('custom_'), I guess. No biggy.

I would go with the latter - having one variable that handles partitioning seems cleaner to me.