Fixed the Bosh::AzureCloud::Cloud.initialize method's signature so …

[Justin-W]

…that the 2nd argument is optional.

Note: Based on the changes made in commit 6c6ec59 (which added the 2nd argument), it seems that the argument was intended to be optional from the start. However, according to the method's signature, it was technically a required argument (despite that the method handles nil/missing values internally on line 29). Also, the following code already doesn't pass the 2nd argument:

• bosh-azure-cpi-release/src/bosh_azure_cpi/bin/bosh_azure_console

  Line 29 in 9754cf8

  @cpi ||= Bosh::AzureCloud::Cloud.new(@config)

Note: The separate handling of the argument's default value on lines 26 and 29 are not redundant. The default value in the signature (line 26) only applies when the optional arg is omitted by the caller. If the caller explicitly specifies the argument with a value of nil, the arg default doesn't override the nil, but the || 1 on line 29 will. And without the argument default on line 26, the argument technically isn't optional, it is required. Meaning that it is technically an error for any caller to omit that argument, and that line 29 would therefore be covering up the caller's misuse of the method. So both "defaults" are needed in order to correctly handle all of the possible method invocation options for that argument.

Here is the error message which was previously being detected (prior to this change):

Missing argument 'api_version'. Required 2, 'options, api_version'

Checklist:

Please check each of the boxes below for which you have completed the corresponding task:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• [ N/A ] I have added tests that prove my fix is effective or that my feature works
• All unit tests pass locally (after my changes)
• Rubocop reports zero offenses (after my changes)

Please include below the summary portions of the output from the following 2 scripts:

pushd src/bosh_azure_cpi
  ./bin/test-unit
  ./bin/rubocop_check
popd

NOTE: Please see how to setup dev environment and run unit tests in docs/development.md.

Unit Test output:

972 examples, 0 failures
Coverage report generated for RSpec to /Users/justinw/go/src/github.com/cloudfoundry/bosh-azure-cpi-release/src/bosh_azure_cpi/coverage. 4182 / 4203 LOC (99.5%) covered.

Rubocop output:

184 files inspected, no offenses detected

Changelog

• Changed the Bosh::AzureCloud::Cloud.initialize method's signature to make the 2nd positional argument explicitly optional

[bgandon]

I think we should not turn the second parameter as having any default value if it never had, and without any requirement for it.

We should also not use any silent defaults that fix mis-use of the code, and also not implement unnecessary defensive measures. So, just conform to the let-it-crash principle.

Let's keep it simple, please only remove the || 1 in @api_version = api_version || 1, update the incorrect calling from bin/bosh_azure_console script, using the current API version, then add some test for better code coverage. So that in the future, some test can detect the issue you've pointed here. Thanks.

[Justin-W]

@bgandon I've revised the PR as you requested:

1. the method now conforms to the let-it-crash principle, raises an error if any invalid value is passed for the argument
2. updated all invocations which were lacking an explicit value for that arg
3. Updated the method's unit specs to test all of the valid invocation options and also the most likely invalid ones

[bgandon]

Okay 😅, I think that's a bit overkill amount of code for the severity of the initial concern to be honest, but I appreciate the tenacity of your commitment in making it right 👍
LGTM.

[rkoster]

LGTM

[rkoster]

Thanks! @Justin-W

[rkoster]

Just for context, this PR has broken some behavior. See f1f495c for more details.