Allow to accept multiple values for --security-group-ids and show deprecated message for comma seprated #439
Conversation
| end | ||
| end | ||
|
|
||
| context "when invalid input is given for --security-group-ids from knife.rb" do |
There was a problem hiding this comment.
is there a reason not support this?
There was a problem hiding this comment.
couldn't we just check if we were given an array?
There was a problem hiding this comment.
@btm We are expecting a input from user as 'x,y,z'. So we are expecting the input should be in same format. If we have to accept the input as an array too then we need to include that also in help description.
There was a problem hiding this comment.
I understand that from the CLI, I wouldn't add array support for the CLI. But using an array would be pretty natural from inside the knife.rb.
There was a problem hiding this comment.
@btm If we do that then we are expecting different input from cli and from knife.rb and its no where mentioned in documentation, so we tried to avoid this confusion for user and expecting same input format from cli and knife.rb
There was a problem hiding this comment.
We should still allow a comma separated list in a string in the configuration file, and use the same code to parse it that we use when we take a comma separated list from the command line.
We should also allow an array in the configuration file. It is ruby, so the user is likely to try to use the array. We allow arrays in other places in the configuration files, like cookbook_path and ohai.disabled_plugins. We don't have to document it in the user documentation (although it wouldn't hurt). I'd expect a test like it "allows --security-group-ids set from an Array in knife.rb".
|
@Vasu1105 I talked to some chef maintainers, and we want to standardize on passing a CLI avalue multiple times by calling the argument multiple times instead of a comma separated list. A) multiple values passed to the CLI is done by passing the flag multiple times This is so we don't have to deal with parsing issues exactly like this. Implementing A:
Implementing B:
|
|
@btm working on required changes. Fixing specs is in progress. |
Vasu1105
force-pushed
the
vj/security_group_ids_fix
branch
from
d204772
to
54be944
Compare
Vasu1105
changed the title
Vasu1105
force-pushed
the
vj/security_group_ids_fix
branch
from
54be944
to
c63f7a1
Compare
|
@btm I have created gist for ec2_server_create_spec since git is not showing the diff as it exceeding the number of line changes REF gist https://gist.github.com/Vasu1105/aba8bf2d883bfd51dd717c4598aff9ee. |
| :long => "--security-group-ids 'X,Y,Z'", | ||
| :description => "The security group ids for this server; required when using VPC,Please provide values in format --security-group-ids 'X,Y,Z'", | ||
| :proc => Proc.new { |security_group_ids| security_group_ids.split(',') } | ||
| :short => "-g SECURITY_GROUP_IDS", |
There was a problem hiding this comment.
this should probable read -g SECURITY_GROUP_ID since it takes a single value.
|
@mwrock & @btm I have updated the changes and also updated the gist for spec file https://gist.github.com/Vasu1105/aba8bf2d883bfd51dd717c4598aff9ee. Please review |
| :long => "--security-group-ids 'X,Y,Z'", | ||
| :description => "The security group ids for this server; required when using VPC,Please provide values in format --security-group-ids 'X,Y,Z'", | ||
| :proc => Proc.new { |security_group_ids| security_group_ids.split(',') } | ||
| :long => "--security-group-ids IDS", |
There was a problem hiding this comment.
I think I'd keep 'X,Y,Z' over IDS
|
👍 |
…ion for --secruity-group-ids
…ed validation to accept array from knife.rb
…se 'let' insted of instance variables
NimishaS
force-pushed
the
vj/security_group_ids_fix
branch
from
a545ac3
to
3091397
Compare
No description provided.