Change long option #2668

sharder996 · 2022-07-18T17:36:20Z

This PR changes the long option for memory allocation of the launch command from --mem to --memory. --mem is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, --memory will take precedence.

Fixes #2490 for original change request.

luis4a0

Hey @sharder996, thanks!

Looks good, hiding the going-to-be-deprecated option seems the best choice to me.

One detail, though: I think that specifying --mem and --memory toghether must make Multipass to fail.

sharder996 · 2022-07-18T18:03:08Z

If both options are used together, then --memory will be used @luis4a0

codecov · 2022-07-18T18:09:10Z

Codecov Report

Merging #2668 (414db12) into main (10e328a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2668      +/-   ##
==========================================
+ Coverage   86.46%   86.48%   +0.01%     
==========================================
  Files         217      217              
  Lines       10819    10833      +14     
==========================================
+ Hits         9355     9369      +14     
  Misses       1464     1464

Impacted Files	Coverage Δ
src/client/cli/cmd/launch.cpp	`85.02% <100.00%> (+0.69%)`	⬆️
src/client/cli/cmd/exec.cpp	`99.05% <0.00%> (+0.02%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

luis4a0 · 2022-07-18T19:13:54Z

@sharder996 it seems you answered before I update my question. I didn't read that --memory takes precedence over --mem in the description, sorry for that.

My point is that, in case the user knows that there is a --memory parameter, we should fail saying that --mem must not be used. The point in leaving the old parameter is to allow for old scripts to be used, but I don't think we should permit using it interchangeably with the new parameter. I'm mentioning this in order to open a discussion, not yet to force a change in your code. What do you think?

townsend2010 · 2022-07-18T19:21:04Z

I think if both --mem and --memory are specified at the same time, that should be a failure since the user would be aware of --memory.

Also, to take this one step further, I think if --mem is only specified, a warning should be printed out saying --mem is being deprecated in favor of --memory and please update any scripts, etc.

sharder996 · 2022-07-18T20:08:38Z

I like Chris' idea as it gives the user some head ups that the option will be having potentially breaking changes in the future rather than a silent change that still works.

luis4a0 · 2022-07-19T13:08:50Z

Hey @sharder996. Thanks, it's much better now!

A tiny bit of coverage remaining and we're done. Can you please add coverage to the error messages?

luis4a0 · 2022-07-20T13:45:57Z

Excellent, thanks @sharder996! A final detail: can you please change "in favor" to "in favour", so we use UK English in the output?

luis4a0

Excellent, thanks!

bors r+

2668: Change long option r=luis4a0 a=sharder996 This PR changes the long option for memory allocation of the `launch` command from `--mem` to `--memory`. `--mem` is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, `--memory` will take precedence. Fixes #2490 for original change request. Co-authored-by: sharder996 <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2022-07-20T18:31:47Z

Timed out.

sharder996 · 2022-07-20T18:38:07Z

bors r+

2667: Linux qemu uefi r=luis4a0 a=andrei-toterman Make QEMU boot using UEFI on Linux fix #1477 2668: Change long option r=sharder996 a=sharder996 This PR changes the long option for memory allocation of the `launch` command from `--mem` to `--memory`. `--mem` is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, `--memory` will take precedence. Fixes #2490 for original change request. Co-authored-by: Andrei Toterman <[email protected]> Co-authored-by: sharder996 <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2022-07-20T19:02:30Z

Build failed (retrying...):

Linux

2668: Change long option r=sharder996 a=sharder996 This PR changes the long option for memory allocation of the `launch` command from `--mem` to `--memory`. `--mem` is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, `--memory` will take precedence. Fixes #2490 for original change request. Co-authored-by: sharder996 <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2022-07-20T22:32:15Z

Build failed:

Linux

luis4a0 · 2022-07-21T11:46:31Z

bors retry

2668: Change long option r=sharder996 a=sharder996 This PR changes the long option for memory allocation of the `launch` command from `--mem` to `--memory`. `--mem` is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, `--memory` will take precedence. Fixes #2490 for original change request. Co-authored-by: sharder996 <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2022-07-21T13:01:45Z

Build failed:

Windows

luis4a0 · 2022-07-21T13:03:51Z

bors retry

2668: Change long option r=sharder996 a=sharder996 This PR changes the long option for memory allocation of the `launch` command from `--mem` to `--memory`. `--mem` is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, `--memory` will take precedence. Fixes #2490 for original change request. Co-authored-by: sharder996 <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2022-07-21T13:28:40Z

Build failed:

Linux

luis4a0 · 2022-07-21T13:29:48Z

bors retry

2668: Change long option r=sharder996 a=sharder996 This PR changes the long option for memory allocation of the `launch` command from `--mem` to `--memory`. `--mem` is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, `--memory` will take precedence. Fixes #2490 for original change request. Co-authored-by: sharder996 <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2022-07-21T13:55:09Z

Build failed:

Linux

luis4a0 · 2022-07-22T18:21:11Z

Weird, as usual

bors retry

2668: Change long option r=sharder996 a=sharder996 This PR changes the long option for memory allocation of the `launch` command from `--mem` to `--memory`. `--mem` is still supported for backwards compatibility, but will not be displayed in the help text. If both options are used, `--memory` will take precedence. Fixes #2490 for original change request. Co-authored-by: sharder996 <[email protected]> Co-authored-by: ScottH <[email protected]>

bors · 2022-07-22T19:11:31Z

Build failed:

Windows

townsend2010 · 2022-07-22T19:47:42Z

We'll try this one more time since the other branch was out of sync...

bors retry

bors · 2022-07-22T20:20:49Z

Build failed:

Linux

change long option

d19e564

luis4a0 reviewed Jul 18, 2022

View reviewed changes

add error msgs

f76adfc

trying out tests

a413b65

update to UK english

414db12

luis4a0 approved these changes Jul 20, 2022

View reviewed changes

townsend2010 merged commit 19b118a into main Jul 22, 2022

bors bot deleted the allow-memory-option branch July 22, 2022 20:22

Change long option #2668

Change long option #2668

Uh oh!

Conversation

sharder996 commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luis4a0 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharder996 commented Jul 18, 2022

Uh oh!

codecov bot commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

luis4a0 commented Jul 18, 2022

Uh oh!

townsend2010 commented Jul 18, 2022

Uh oh!

sharder996 commented Jul 18, 2022

Uh oh!

luis4a0 commented Jul 19, 2022

Uh oh!

luis4a0 commented Jul 20, 2022

Uh oh!

luis4a0 left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Jul 20, 2022

Uh oh!

sharder996 commented Jul 20, 2022

Uh oh!

bors bot commented Jul 20, 2022

Uh oh!

bors bot commented Jul 20, 2022

Uh oh!

luis4a0 commented Jul 21, 2022

Uh oh!

bors bot commented Jul 21, 2022

Uh oh!

luis4a0 commented Jul 21, 2022

Uh oh!

bors bot commented Jul 21, 2022

Uh oh!

luis4a0 commented Jul 21, 2022

Uh oh!

bors bot commented Jul 21, 2022

Uh oh!

luis4a0 commented Jul 22, 2022

Uh oh!

bors bot commented Jul 22, 2022

Uh oh!

townsend2010 commented Jul 22, 2022

Uh oh!

bors bot commented Jul 22, 2022

Uh oh!

Uh oh!

sharder996 commented Jul 18, 2022 •

edited

Loading

luis4a0 left a comment •

edited

Loading

codecov bot commented Jul 18, 2022 •

edited

Loading