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

feat(terraform): add CKV_NCP_1 about lb target group health check, CKV_NCP_2 about access control group description #3569

Merged
merged 16 commits into from
Oct 2, 2022

Conversation

Floodnut
Copy link
Contributor

@Floodnut Floodnut commented Sep 27, 2022

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Description

  • Add new terraform provider Naver Cloud Platform.
    -- CKV_NCP_1 is associated with Loadbalancing target group's health check.
    -- CKV_NCP_2 is associated with Access Control Group Description.

Description

https://registry.terraform.io/providers/NaverCloudPlatform/ncloud/latest/docs/resources/lb_target_group
https://registry.terraform.io/providers/NaverCloudPlatform/ncloud/latest/docs/resources/access_control_group

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my feature, policy, or fix is effective and works
  • New and existing tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@gruebel gruebel changed the title feat(terraform): add CKV_NCP_1 about NCP lb target group health check, CKV_NCP_2 about acccess control group description feat(terraform): add CKV_NCP_1 about lb target group health check, CKV_NCP_2 about access control group description Sep 27, 2022
@gruebel gruebel changed the title feat(terraform): add CKV_NCP_1 about lb target group health check, CKV_NCP_2 about access control group description feat(terraform): add CKV_NCP_1 about lb target group health check, CKV_NCP_2 about access control group description Sep 27, 2022
Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

nice job 🥇 , can you adjust the tests to have dedicated TF files and a test file to simulate an actual run. You can check this PR on how it should look like. If you have problems, let me know, then I can help you out 🙂

@pj991207
Copy link
Contributor

n you adjust the tests to have dedicated TF files and a test file to simulate an actual run.

did you say that making test code like this snapshot?
image
image

@pj991207
Copy link
Contributor

that making test code like this snapshot?

I made a test code like that
image
image

@gruebel
Copy link
Contributor

gruebel commented Sep 28, 2022

n you adjust the tests to have dedicated TF files and a test file to simulate an actual run.

did you say that making test code like this snapshot? image image

yeah correct. how you wrote the tests is the old way of doing it and it resulted in some issues we had. New tests should use the style I described and over time we update the old tests to fit the newer style.

@Floodnut Floodnut changed the title feat(terraform): add CKV_NCP_1 about lb target group health check, CKV_NCP_2 about access control group description feat(terraform): add CKV_NCP_1 about lb target group health check, CKV_NCP_2 about access control group description Sep 28, 2022
@Floodnut
Copy link
Contributor Author

We change our test codes with CONTRIBUTING.md styles.
Can you check we commited and what we missed?
thx.

Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

thanks looks great. would be nice, if you can the mentioned resource to be tested too 🙂

@@ -0,0 +1,10 @@
resource "ncloud_access_control_group" "pass" {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you also add a passing + failing resource for ncloud_access_control_group_rule

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'll check additional resource ncloud_access_control_group_rule!! thanks!

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 add resource ncloud_access_control_group_rule !

And Does resources to be tested mean what we commited?

Copy link
Contributor

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

great work 🍻

@gruebel gruebel merged commit 71a4d22 into bridgecrewio:master Oct 2, 2022
losisin pushed a commit to ignite-analytics/checkov that referenced this pull request Oct 2, 2022
…V_NCP_2 about access control group description (bridgecrewio#3569)

* [22.09.27][추가] CKV_NCP_1

* [22.09.27][추가] CKV_NCP_2

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Create main.yml

* [22.09.28][수정] Lint test

* Delete main.yml

* [22.09.29][수정]testcode 수정

* [22.09.29][수정] 테스트 코드 수정

* [22.09.29][수정] 테스트코드 수정

* [22.09.29][수정] add test resource for 'ncloud_access_control_group_rule'

Co-authored-by: pj991207 <[email protected]>
Co-authored-by: Anton Grübel <[email protected]>
losisin pushed a commit to ignite-analytics/checkov that referenced this pull request Oct 2, 2022
…V_NCP_2 about access control group description (bridgecrewio#3569)

* [22.09.27][추가] CKV_NCP_1

* [22.09.27][추가] CKV_NCP_2

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Create main.yml

* [22.09.28][수정] Lint test

* Delete main.yml

* [22.09.29][수정]testcode 수정

* [22.09.29][수정] 테스트 코드 수정

* [22.09.29][수정] 테스트코드 수정

* [22.09.29][수정] add test resource for 'ncloud_access_control_group_rule'

Co-authored-by: pj991207 <[email protected]>
Co-authored-by: Anton Grübel <[email protected]>
losisin pushed a commit to ignite-analytics/checkov that referenced this pull request Oct 2, 2022
…V_NCP_2 about access control group description (bridgecrewio#3569)

* [22.09.27][추가] CKV_NCP_1

* [22.09.27][추가] CKV_NCP_2

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Apply suggestions from code review

Co-authored-by: Anton Grübel <[email protected]>

* Create main.yml

* [22.09.28][수정] Lint test

* Delete main.yml

* [22.09.29][수정]testcode 수정

* [22.09.29][수정] 테스트 코드 수정

* [22.09.29][수정] 테스트코드 수정

* [22.09.29][수정] add test resource for 'ncloud_access_control_group_rule'

Co-authored-by: pj991207 <[email protected]>
Co-authored-by: Anton Grübel <[email protected]>
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.

4 participants