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.

Sign up for GitHub

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

Jump to bottom

resource/ssm_patch_baseline: Add compliance level to patch approval rules #1531

Merged
merged 3 commits into from
Sep 4, 2017
Merged

resource/ssm_patch_baseline: Add compliance level to patch approval rules #1531

merged 3 commits into from
Sep 4, 2017

Conversation

pmorton
Copy link
Contributor

@pmorton pmorton commented Aug 29, 2017 •
edited
Loading

Hi All - This pull request includes an update to support per patch rule compliance levels.

  • Documentation has been updated
  • Acceptance tests have been updated and run on the SSM provider

Please let me know if you need anything else to get this merged in. 🍻

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 31, 2017
Ninir
Ninir suggested changes Aug 31, 2017
View reviewed changes
Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Hi @pmorton

Looks good to me, just left 2 comments to address! :)

aws/resource_aws_ssm_patch_baseline.go Outdated
"compliance_level": {
Type: schema.TypeString,
Optional: true,
Default: "UNSPECIFIED",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use the constants here and the line after? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to move all of the levels and operating systems into constants. That should be contentious. I also added a constant-like variable to encapsulate the list of acceptable values. I am happy to revert this if needed....

aws/resource_aws_ssm_patch_baseline.go
@@ -306,6 +313,7 @@ func expandAwsSsmPatchRuleGroup(d *schema.ResourceData) *ssm.PatchRuleGroup {
rule := &ssm.PatchRule{
ApproveAfterDays: aws.Int64(int64(rCfg["approve_after_days"].(int))),
PatchFilterGroup: filterGroup,
ComplianceLevel: aws.String(rCfg["compliance_level"].(string)),
Copy link
Contributor

Choose a reason for hiding this comment

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

You should check that the value is set here before assigning it. Won't it break otherwise?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Ninir - There might be a gap in my knowledge so let me explain my thinking. compliance_level has a default value of UNSPECIFIED (Schema below). Since there is a default value, I would expect that either the user has provided the value in which case it is set to the user's input or the user has not provided the value which means that it is set to the default. If the above is true, then the value should always be initialized. Is there another path or gap in my knowledge that could lead to an uninitialized value?

						"compliance_level": {
							Type:         schema.TypeString,
							Optional:     true,
							Default:      "UNSPECIFIED",
							ValidateFunc: validation.StringInSlice([]string{"CRITICAL", "HIGH", "MEDIUM", "LOW", "INFORMATIONAL", "UNSPECIFIED"}, false),
						}

Copy link
Contributor

Choose a reason for hiding this comment

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

You're totally right, missed the defaultFunc, nevermind :)

pmorton
pmorton commented Aug 31, 2017
View reviewed changes
Copy link
Contributor Author

@pmorton pmorton left a comment

Choose a reason for hiding this comment

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

@Ninir Responded to the code review. 1 change and one additional comment.

aws/resource_aws_ssm_patch_baseline.go
@@ -306,6 +313,7 @@ func expandAwsSsmPatchRuleGroup(d *schema.ResourceData) *ssm.PatchRuleGroup {
rule := &ssm.PatchRule{
ApproveAfterDays: aws.Int64(int64(rCfg["approve_after_days"].(int))),
PatchFilterGroup: filterGroup,
ComplianceLevel: aws.String(rCfg["compliance_level"].(string)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Ninir - There might be a gap in my knowledge so let me explain my thinking. compliance_level has a default value of UNSPECIFIED (Schema below). Since there is a default value, I would expect that either the user has provided the value in which case it is set to the user's input or the user has not provided the value which means that it is set to the default. If the above is true, then the value should always be initialized. Is there another path or gap in my knowledge that could lead to an uninitialized value?

						"compliance_level": {
							Type:         schema.TypeString,
							Optional:     true,
							Default:      "UNSPECIFIED",
							ValidateFunc: validation.StringInSlice([]string{"CRITICAL", "HIGH", "MEDIUM", "LOW", "INFORMATIONAL", "UNSPECIFIED"}, false),
						}

aws/resource_aws_ssm_patch_baseline.go Outdated
"compliance_level": {
Type: schema.TypeString,
Optional: true,
Default: "UNSPECIFIED",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code to move all of the levels and operating systems into constants. That should be contentious. I also added a constant-like variable to encapsulate the list of acceptable values. I am happy to revert this if needed....

Ninir
Ninir reviewed Sep 1, 2017
View reviewed changes
aws/resource_aws_ssm_patch_baseline.go Outdated
ssmPatchComplianceLevelMedium = "MEDIUM"
ssmPatchComplianceLevelHigh = "HIGH"
ssmPatchComplianceLevelCritical = "CRITICAL"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of the existing ones, provided by the SDK: https://github.com/terraform-providers/terraform-provider-aws/blob/master/vendor/github.com/aws/aws-sdk-go/service/ssm/api.go#L27668-L27686 😄

and for patch OS: https://github.com/terraform-providers/terraform-provider-aws/blob/master/vendor/github.com/aws/aws-sdk-go/service/ssm/api.go#L27615-L27627

Could you make the change? last thing remaining I think! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ninir Done!

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAWSSSMPatchBaseline* -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSSSMPatchBaseline_basic
--- PASS: TestAccAWSSSMPatchBaseline_basic (23.50s)
=== RUN   TestAccAWSSSMPatchBaselineWithOperatingSystem
--- PASS: TestAccAWSSSMPatchBaselineWithOperatingSystem (22.57s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	46.102s

Ninir
Ninir approved these changes Sep 4, 2017
View reviewed changes
Copy link
Contributor

@Ninir Ninir left a comment

Choose a reason for hiding this comment

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

Looks veery good to me @pmorton !

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSSMPatchBaseline'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSSMPatchBaseline -timeout 120m
=== RUN   TestAccAWSSSMPatchBaseline_basic
--- PASS: TestAccAWSSSMPatchBaseline_basic (21.92s)
=== RUN   TestAccAWSSSMPatchBaselineWithOperatingSystem
--- PASS: TestAccAWSSSMPatchBaselineWithOperatingSystem (22.34s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	44.296s

Thanks for the work! 👍

@Ninir Ninir changed the title Add compliance level to patch approval rules resource/ssm_patch_baseline: Add compliance level to patch approval rules Sep 4, 2017
@Ninir Ninir merged commit e5bd586 into hashicorp:master Sep 4, 2017
nbaztec pushed a commit to nbaztec/terraform-provider-aws that referenced this pull request Sep 26, 2017
@pmorton @Ninir
resource/ssm_patch_baseline: Add compliance level to patch approval r…
a3741bc 
…ules (hashicorp#1531)

resource/ssm_patch_baseline: Add compliance level to patch approval rules
@ghost
Copy link

ghost commented Apr 11, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ninir Ninir Ninir approved these changes

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pmorton @Ninir