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

Adding attributes for ELB resource #623

Merged

Conversation

Rohit1509
Copy link

@Rohit1509 Rohit1509 commented Sep 11, 2021

Signed-off-by: Rohit Joshi rohit.prasad.joshi@sap.com

Description

Adding new attributes for ELB resource

Issues Resolved

None

Check List

Please fill box or appropriate ([x]) or mark N/A.

Signed-off-by: Rohit Joshi <rohit.prasad.joshi@sap.com>
@Rohit1509 Rohit1509 requested review from a team as code owners September 11, 2021 01:01
@@ -43,6 +43,10 @@ def initialize(opts = {})
policies_in_use = resp.listener_descriptions.map(&:policy_names).flatten.uniq
@ssl_policies = @aws.elb_client.describe_load_balancer_policies(load_balancer_name: opts[:load_balancer_name])
.policy_descriptions.select { |p| policies_in_use.include?(p.policy_name) }
@cross_zone_load_balancing = @aws.elb_client.describe_load_balancer_attributes(load_balancer_name: opts[:load_balancer_name])
Copy link
Contributor

Choose a reason for hiding this comment

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

@Rohit1509 , Could we please do the n/w call to describe_load_balancer_attributes only once and set both the attributes using the attributes received ?

Also a couple of recommendations would be to

  • lazy load this only when the method is called
  • since this returns boolean flag, please try using matchers syntax for this ..
    for example,

you can implement methods
cross_zone_load_balancing_enabled? & access_log_enabled?
which will be available in inspec controls as

it { should be_cross_zone_load_balancing_enabled }
it { should be_access_log_enabled }

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Signed-off-by: Rohit Joshi <rohit.prasad.joshi@sap.com>
Copy link
Contributor

@sathish-progress sathish-progress left a comment

Choose a reason for hiding this comment

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

LGTM

@sathish-progress sathish-progress added CFT Support Documentation Version: Bump Minor Used by github.minor_bump_labels to bump the Minor version number. labels Sep 14, 2021
|listeners | A collection of the listeners for the load balancer. |
|ssl_policies | A collection of the SSL Policies configured in-use for the load balancer (and their policy attributes). |
|protocols | A list of the protocols configured for the listeners of the load balancer. |
|cross\_zone\_load\_balancing\_enabled? | The cross zone load balancing status for ELB. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
|cross\_zone\_load\_balancing\_enabled? | The cross zone load balancing status for ELB. |
|cross\_zone\_load\_balancing\_enabled? | The cross-zone load balancing status for ELB. |

Copy link
Author

Choose a reason for hiding this comment

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

fixed


## Examples

##### Test that cross zone load balancing for ELB is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
##### Test that cross zone load balancing for ELB is enabled
##### Test that cross-zone load balancing for ELB is enabled

Copy link
Author

Choose a reason for hiding this comment

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

fixed

it { should be_cross_zone_load_balancing_enabled }
end

##### Test that access log for ELB is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
##### Test that access log for ELB is enabled
##### Test that access logs for ELB are enabled

Copy link
Author

Choose a reason for hiding this comment

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

fixed

Rohit Joshi added 2 commits September 24, 2021 17:33
Signed-off-by: Rohit Joshi <rohit.prasad.joshi@sap.com>
@soumyo13
Copy link
Contributor

@IanMadd Can you please approve if the review is done?

Copy link
Contributor

@IanMadd IanMadd left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Sep 30, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@sathish-progress sathish-progress merged commit 97f86cd into inspec:main Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CFT Support Documentation Version: Bump Minor Used by github.minor_bump_labels to bump the Minor version number.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants