Allow changing whether a distribution group is public. by anywherepilot · Pull Request #808 · microsoft/appcenter-cli

anywherepilot · 2020-04-16T08:59:51Z

Add public and private switches to distribute groups update. These allow making a group public or the opposite.

Resolves #642.

jp-andre · 2020-04-17T05:07:34Z

src/commands/distribute/groups/update.ts

+  @shortName("p")
+  @longName("isPublic")
+  @hasArg
+  public isPublic: string;


public or maybe is-public though the is is not relevant.
Do we natively support bool here by any chance?

Also it looks like this should act like a switch, where -p (or even --public) alone should set the value to true.

I considered that, but then how do you set it to not public? With another switch?

--public=false

we apparently have support for booleans in the base class, let's see what it can do, and whether we need to expand it for this (which would not be worth the effort)

Alternatively, a common convention is to prefix with no as in --no-public (no short form here)

We're using names like isMandatory and isDisabled in other places, but I can rename it to just public. We also have precedent for that. It's a mix.

This is not a flag, it's a setter. It's setting a value to true or false, not on the command but on the distribution group it's modifying.

Problem with making it bool is that it will need to default to false, and we'll no longer be able to check whether it is passed or not. TypeScript doesn't support nullable booleans. We can't work with that, because we can't just go set the value based on that information. Since it's a setter, not a flag.

If providing a string value for a boolean is really unwanted, we can have two parameters called makePublic and makePrivate or something. I'm not sure that looks nicer.

We're using names like isMandatory and isDisabled in other places, but I can rename it to just public. We also have precedent for that. It's a mix.

Sorry, but that's not true. From the code right there:

@help("Specifies whether this release should be considered mandatory. (Putting -m flag means mandatory)") @shortName("m") @longName("mandatory") public isMandatory: boolean;

The option is called mandatory and it's a boolean switch. Specifically for casing, the only mistake I could find was displayKeys (we may want to consider fixing that in another PR, it should be display-keys).

This is not a flag, it's a setter. It's setting a value to true or false, not on the command but on the distribution group it's modifying.

I understand that. There are ways to make this work. Either the option could also accept an optional argument, or we could use something like --no-public or --private as you proposed.

Problem with making it bool is that it will need to default to false, and we'll no longer be able to check whether it is passed or not. TypeScript doesn't support nullable booleans. We can't work with that, because we can't just go set the value based on that information. Since it's a setter, not a flag.

The default value of a boolean field is "the field does not even exist", and "undefined" is perfectly acceptable for a boolean in TS. Yay.

The only question is how much effort is it to have an optional argument to a boolean switch, not whether it's possible or impossible.

One more vote for having it as boolean using undefined. Otherwise we should handle the string to be a boolean value and that sound like more work.

You are right on the naming. I was looking at the variable names, not the argument names.

The way booleans work in the CLI is by not setting the hasArg attribute on a property. It then becomes a boolean (regardless of what you say it should be in the property definition). The value is false when not provided. There is no undefined. Unless we add support for an optional parameter, which would be difficult*, there is mathematically speaking no difference between false and undefined. It's just the "not true" option, and cannot be used. So we need two switches.

It would require defining an actual switch type parameter with a new syntax for taking an optional false argument.

There is no other example that I know of in the CLI that sets a boolean as obviously as this one, so we have to make a new decision. You and @yujincat are in favor of having two switches (or against a parameter taking a "true"/"false" string), and I don't have an opinion. So let's go with the two switches. I'll call them --public and --no-public as you suggested.

This is a good discussion btw, I'm glad we're having it :)
Things may be built on top of this in the future.

test/commands/distribute/groups/update-test.ts

src/commands/distribute/groups/update.ts

yujincat · 2020-04-21T03:42:09Z

src/commands/distribute/groups/update.ts

+  @shortName("p")
+  @longName("isPublic")
+  @hasArg
+  public isPublic: string;


One more vote for having it as boolean using undefined. Otherwise we should handle the string to be a boolean value and that sound like more work.

src/commands/distribute/groups/update.ts

jp-andre

Since you've chosen the 2 switches option and started the code implementation, let's use that.

src/commands/distribute/groups/update.ts

The other name is incompatible with the way we initialize minimist. Add and refactor tests.

yujincat · 2020-04-22T04:57:15Z

test/commands/distribute/groups/update-test.ts

-  function testCommandFailure(executionScope: Nock.Scope, abortScope?: Nock.Scope) {
-    expect(abortScope.isDone()).to.eql(false, "Unexpected requests were made");
+  function testCommandFailure(executionScope: Nock.Scope, skippedScope?: Nock.Scope) {
+    expect(skippedScope.isDone()).to.eql(false, "Unexpected requests were made");


Why doesn't it have null check like testCommandSuccess?

A better question would be why is this method copied into every test class 😛
I didn't need to fix this one to make my code work. Let's fix them all at once later. Thanks for your review.

yujincat

Approve with suggestion

Add isPublic option to distribute groups update.

9d489fc

jp-andre reviewed Apr 17, 2020

View reviewed changes

Add test

10b46d5

yujincat reviewed Apr 21, 2020

View reviewed changes

Turn --public parameter into switch and add --no-public.

9664b90

yujincat reviewed Apr 21, 2020

View reviewed changes

src/commands/distribute/groups/update.ts Show resolved Hide resolved

jp-andre reviewed Apr 21, 2020

View reviewed changes

src/commands/distribute/groups/update.ts Show resolved Hide resolved

Rename --no-public switch to --private.

962dfe7

The other name is incompatible with the way we initialize minimist. Add and refactor tests.

anywherepilot changed the title ~~Add isPublic option to distribute groups update.~~ Allow changing whether a distribution group is public. Apr 21, 2020

anywherepilot marked this pull request as ready for review April 21, 2020 06:41

yujincat reviewed Apr 22, 2020

View reviewed changes

yujincat approved these changes Apr 22, 2020

View reviewed changes

anywherepilot merged commit c795fe6 into master Apr 22, 2020

anywherepilot deleted the jerietve/group-set-is-public branch April 22, 2020 05:36

enterstudio mentioned this pull request May 14, 2022

[Snyk] Security upgrade inquirer from 1.2.3 to 6.5.1 enterstudio/mobile-center-cli#14

Open

Conversation

anywherepilot commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jp-andre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anywherepilot Apr 22, 2020 • 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!

yujincat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

anywherepilot commented Apr 16, 2020 •

edited

Loading

anywherepilot Apr 22, 2020 •

edited

Loading