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

support --scale #822

Closed
duglin opened this issue May 1, 2020 · 25 comments · Fixed by #1114
Closed

support --scale #822

duglin opened this issue May 1, 2020 · 25 comments · Fixed by #1114
Assignees
Labels
kind/feature New feature or request
Milestone

Comments

@duglin
Copy link
Contributor

duglin commented May 1, 2020

I find myself often needing to do: --min-scale=X --max-scale=x, usually when x=1.

It might be nice to support a short-cut here: --scale=x to mean "set both min & max".

Just a thought...

@duglin duglin added the kind/feature New feature or request label May 1, 2020
@duglin duglin changed the title support --scale support --scale May 1, 2020
@duglin duglin changed the title support --scale support --scale May 1, 2020
coryrc pushed a commit to coryrc/client that referenced this issue May 14, 2020
@mattmoor
Copy link
Member

It could support a range as well, e.g. 0-10

@rhuss
Copy link
Contributor

rhuss commented May 19, 2020

So maybe deprecating min-scale and max-scale and shoot for a

--scale 1      min-scale == max-sclae == 1 (same as --scale 1..1)
--scale 1..    min-scale == 1, max-scale undefined
--scale ..5    min-scale undefined, max-scale == 5 (same as --scale 0..5)
--scale 2..5   min-scale == 2, max-scale ==5

@duglin
Copy link
Contributor Author

duglin commented May 20, 2020

I think supporting a range is an ok option, and would prefer “-“ over “..”, but I wouldn’t remove the min and max options though. Doing that would require people to do a get() of some kind to find the current values before they could change just one of them. Meaning, if I don’t know what the current min and max are, but I know I want to set min to zero, I would need to first get() the ksvc to get the max and then do 0-max, where it would be nicer to just do —min-scale=0

@rhuss
Copy link
Contributor

rhuss commented May 20, 2020

@duglin With "undefined" in the example above I mean: Leave untouched.

In you example, just specify --scale 0..

@rhuss
Copy link
Contributor

rhuss commented May 20, 2020

I don't like - very much because a scale of "-5" is ambigous (and nonsense if interpreted as a sign). "-" is too overloaded IMO. .. as the "range" operator is a good fit imo.

@duglin
Copy link
Contributor Author

duglin commented May 20, 2020

re: "undefined"... ah, I missed that. Interesting. My first reaction was that it meant "max" (or "min") based on which side it was on. I wonder if other people will jump to the same incorrect conclusion.

re: "-", doi! totally forgot that "-" would be confused with options processing.

@mpetason
Copy link
Contributor

/assign

I'm going to work on a single value for now and then maybe range after I get that working.

@mpetason
Copy link
Contributor

@rhuss Do we want to remove the options for --max/--min and replace it with scale with a range? I haven't done enough work on CLI stuff to know if that will upset users who like the --max/--min functionality instead of ranges with one command.

If we're down to depreciate min/max scale and just use scale then I can start that work next. Should we discuss this in the client channel or would it be better to have a PR going to see what it looks like?

@duglin
Copy link
Contributor Author

duglin commented Jul 15, 2020

my 2 cents...

  • I'm ok with --scale 0-10 since I'm pretty sure that most people can easily figure out what this means
  • I'm less clear on --scale ..10 because I'm not convinced that we won't have to explain this syntax to people over and over.

To me, --max-scale 10 and --scale ..10 are pretty close but the former doesn't require docs.

Is the 2nd syntax solving a pain point?

@navidshaikh
Copy link
Collaborator

IMO, we should keep --max-scale and --min-scale around while --scale serves for cases where both knobs should point to a single value (and also allowing users to use the range thing if they like). Ofcourse this brings additional handling where user should be prompted if these flags are given together.

@rhuss
Copy link
Contributor

rhuss commented Jul 16, 2020

Ok, I see. People love explicit max/min scale ;-), but maybe we can rename them to be close together on the help page. Therefor I suggest:

  • Add a --scale-min
  • Add a --scale-max
  • Add a deprecation warning to --min-scale and --max-scale in the help message and when used, pointing it to the new naming.
  • We don't touch --scale and keep it with a single value (also I still thing having the usual 'range' operator like ".." or maybe even "-" with two explicit boundaries still would be a nice UX).

That way we can keep it compatible for now but would remove then --max-scale and --min-scale in kn 0.1

wdyt ?

@dsimansk
Copy link
Contributor

dsimansk commented Jul 16, 2020

@rhuss do we even need to introduce --scale-min and --scale-max and deprecate the former ones? (adding to the list of flags)

I do like an idea of --scale flag with a range support though, it feels like after a while of bake-in time users would get used to it and min-scale,max-scale could be removed in favour of single scale flag.

@rhuss
Copy link
Contributor

rhuss commented Jul 16, 2020

@dsimansk I think the opinions differ here, whether 3 options should be used or only a single one. Technically even --scale was not needed, so it's all about user convenience. I suggest I open a poll with three options:

  1. Use --scale-min, --scale-max and a single valued --scale for specifying min-scale=max-scale
  2. Use --scale which can carry a scalar value to set min+max scale, a range for setting min and max scale differently or only one of min or max scale (see suggestion above)
  3. Implement both 1. and 2. as they are both compatible.

This poll would then be open until next WG call.

@rhuss
Copy link
Contributor

rhuss commented Jul 16, 2020

Here's the poll. Please vote only once with 👍 for your favourite option. The poll is open until Tuesday.

@rhuss
Copy link
Contributor

rhuss commented Jul 16, 2020

[A]: Use --scale-min, --scale-max and a single valued --scale for specifying min-scale=max-scale

Error on conflicting specification (like it is now already).

@rhuss
Copy link
Contributor

rhuss commented Jul 16, 2020

[B]: Use --scale only which can carry a scalar value to set min+max scale, a range for setting min and max scale differently or only one of min or max scale:

  • 5 : min = max = 5
  • 1..5 : min = 1, max = 5
  • ..5 : min unspecified, max = 5
  • 1.. : min = 1, max unspecified

This example uses the range operator ("..") as known from various programming languages, but the concrete value is not part of this poll option (i.e. can still be discussed)

@rhuss
Copy link
Contributor

rhuss commented Jul 16, 2020

[C]: Implement both [A] and [B] to cover beginner and expert user at the same time

@rhuss
Copy link
Contributor

rhuss commented Jul 16, 2020

In any case, I sugesst to deprecate --max-scale and --min-scale in favor of a more consistent naming.

@mpetason
Copy link
Contributor

I'm working on [B] still - been busy trying to help close out some issues for Docs before release.

@mpetason
Copy link
Contributor

@rhuss @maximilien With the recent PR I'm ignoring 0 values. Is it expected that you should use --scale-min 0 to set the min scale to 0? Right now with create or update, if the value is 0 it either doesn't set it, or it doesn't update it. Something like update --scale 0..5 won't set the min scale to 0, it will ignore the 0 value and just leave whatever is currently set for the min scale annotation. Using create --scale 0..5 would result in not setting a scale-min value, which is equivalent to scale to 0 (if I'm correct about how it is setup without defining it.)

@duglin
Copy link
Contributor Author

duglin commented Oct 27, 2020

Ignoring 0 feels wrong

@mpetason
Copy link
Contributor

@duglin do you have any suggestions about how you would like to see it work? I posted on the issue to get feedback about how it should work so that I can implement it.

@duglin
Copy link
Contributor Author

duglin commented Oct 28, 2020

I’m confused, why not just accept and use 0? Why would we ignore it. @rhuss suggested that ..X be used to mean set a max of X but leave the min untouched.

@duglin
Copy link
Contributor Author

duglin commented Oct 29, 2020

To add more - keep it simple - for x..y:

  • set min to x
  • set max to y
  • generate an error if y < x, or if x < 0

so, yes 0..0 is valid since even today we support --scale-min=0 --scale-max=0

@rhuss
Copy link
Contributor

rhuss commented Oct 30, 2020

@mpetason Sorry, I'm a bit late to the game (such a busy week it is), but I summarized my thinking in #1070 (comment)

The problem I see with the interpretation of using "unchanged" is, that we never have a way to reset a scale-max once it's set. But let discuss over there at the PR ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants