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

Added chef_service_interval option and moved encrypted_data_bag_secret option from public_config to private_config for chef_extension. #408

Merged
merged 3 commits into from Nov 9, 2016

Conversation

Aliasgar16
Copy link

In-progress.

Signed-off-by: aliasgar16 aliasgar.batterywala@msystechnologies.com

aliasgar16 added 2 commits November 1, 2016 18:44
…t option from public_config to private_config for chef_extension.

Signed-off-by: aliasgar16 <aliasgar.batterywala@msystechnologies.com>
…RM template and added the corresponding RSpecs.

Signed-off-by: aliasgar16 <aliasgar.batterywala@msystechnologies.com>
@Aliasgar16 Aliasgar16 force-pushed the ali/integrate_chef_extension_changes branch from 8fefa29 to d072bfc Compare November 3, 2016 07:31
@Aliasgar16
Copy link
Author

Done.

Copy link
Contributor

@btm btm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple nit-picks, but 👍

:long => "--chef-service-interval INTERVAL",
:description => "Optional. It specifies the frequency (in minutes) at which the chef-service runs.
Pass 0 if you don't want the chef-service to be installed on the target machine."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ git grep BootstrapOptions
lib/chef/knife/azure_server_create.rb:      include Knife::Bootstrap::BootstrapOptions
lib/chef/knife/azurerm_server_create.rb:      include Knife::Bootstrap::CommonBootstrapOptions
lib/chef/knife/bootstrap/bootstrap_options.rb:      module BootstrapOptions
lib/chef/knife/bootstrap/common_bootstrap_options.rb:      module CommonBootstrapOptions
lib/chef/knife/bootstrap_azure.rb:      include Knife::Bootstrap::CommonBootstrapOptions
lib/chef/knife/bootstrap_azurerm.rb:      include Knife::Bootstrap::CommonBootstrapOptions

It seems like BootstrapOptions vs CommonBootstrapOptions has gotten out of hand. Although I realize that these options are slightly different as cloud-api appears to optional for ASM but not ARM. So maybe nothing to do here, but this "smells."

Can you at least add comments to the top of BootstrapOptions and CommonBootstrapOptions to describe how they're supposed to be different?

extension = ""
template["resources"].each do |resource|
extension = resource if resource["type"] == "Microsoft.Compute/virtualMachines/extensions"
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to set extension to an empty string here? If it remains nil, wouldn't the expect statements fail either way?

extension = ""
template["resources"].each do |resource|
extension = resource if resource["type"] == "Microsoft.Compute/virtualMachines/extensions"
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about the need to set extension to an empty string.

Signed-off-by: aliasgar16 <aliasgar.batterywala@msystechnologies.com>
@Aliasgar16
Copy link
Author

@btm I have fixed the review comments. Please check and let me know your feedback.

@btm btm merged commit eec3e29 into master Nov 9, 2016
@btm btm deleted the ali/integrate_chef_extension_changes branch November 9, 2016 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants