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

New Resource: wiz_report_graph_query #184

Merged
merged 33 commits into from
Dec 12, 2023
Merged

Conversation

cvirtucio
Copy link
Contributor

@cvirtucio cvirtucio commented Dec 5, 2023

This PR adds a new resource for the Report object of type GRAPH_QUERY.

closes #185

@cvirtucio cvirtucio marked this pull request as ready for review December 5, 2023 21:21
@cvirtucio cvirtucio requested a review from a team as a code owner December 5, 2023 21:21
@cvirtucio
Copy link
Contributor Author

I'm not entirely sure how to address this failure, but I think it could be that I don't have access to the required token.

@jschoombee
Copy link
Collaborator

I'm not entirely sure how to address this failure, but I think it could be that I don't have access to the required token.

Hey @cvirtucio 👋

Thanks for the contribution, I'll make some time hopefully this week or weekend to go through the PR. Regarding the CI failure, it was a recently introduced check and suspect it is indeed a security measure. Its not blocking but it should be fixed.

I'll go through it in some more detail, in the meantime:

  • why was resourceWizReportDelete was split out into resource_report.go? Could we add the function and structs to the main resource_report_graph_query.go?
  • please add examples (hcl) and run go generate ./..., this will generate the docs with examples for the registry, also add import examples should that work as expected for existing saved reports
  • any chance you could add acceptance testing as per contributing guide?

Looking good overall, been meaning to get to the open PRs will try get through them most likely over the weekend.

@cvirtucio
Copy link
Contributor Author

cvirtucio commented Dec 6, 2023

Hi, thanks for giving it a look @jschoombee.

* why was `resourceWizReportDelete` was split out into resource_report.go? Could we add the function and structs to the main resource_report_graph_query.go?

I originally had them all in resource_report.go, but split it off to match the other resources' convention. I kept the Delete func since it seemed generic enough to be used on the other Report types.

* please add examples (hcl) and run `go generate ./...`, this will generate the docs with examples for the registry, also add import examples should that work as expected for existing saved reports

Added example and docs: 5927b92

* any chance you could add acceptance testing as per [contributing guide](https://github.com/AxtonGrams/terraform-provider-wiz/blob/main/_about/CONTRIBUTING.md)?

Added test: dacd275. Do I have to run these tests on our own Wiz environment?

@jschoombee
Copy link
Collaborator

Added test: dacd275. Do I have to run these tests on our own Wiz environment?

thanks, please reference one of the merged PRs, you generally set env vars WIZ_URL WIZ_AUTH_CLIENT_ID and WIZ_AUTH_CLIENT_SECRET targeting your own environment and then share the test results on the PR description

@cvirtucio
Copy link
Contributor Author

Added test: dacd275. Do I have to run these tests on our own Wiz environment?

thanks, please reference one of the merged PRs, you generally set env vars WIZ_URL WIZ_AUTH_CLIENT_ID and WIZ_AUTH_CLIENT_SECRET targeting your own environment and then share the test results on the PR description

Done:

christopher.virtucio@KGYF7CQGCT terraform-provider-wiz % go test -run TestAccResourceWizReportGraphQuery_basic ./...
?   	wiz.io/hashicorp/terraform-provider-wiz	[no test files]
?   	wiz.io/hashicorp/terraform-provider-wiz/internal	[no test files]
?   	wiz.io/hashicorp/terraform-provider-wiz/internal/config	[no test files]
?   	wiz.io/hashicorp/terraform-provider-wiz/internal/utils	[no test files]
?   	wiz.io/hashicorp/terraform-provider-wiz/internal/wiz	[no test files]
ok  	wiz.io/hashicorp/terraform-provider-wiz/internal/acceptance	9.074s
ok  	wiz.io/hashicorp/terraform-provider-wiz/internal/client	(cached) [no tests to run]
ok  	wiz.io/hashicorp/terraform-provider-wiz/internal/provider	(cached) [no tests to run]
christopher.virtucio@KGYF7CQGCT terraform-provider-wiz %

We can't run all of them because we don't have some of the integrations, like ServiceNow.

.github/workflows/pull.yml Show resolved Hide resolved
internal/provider/resource_report_graph_query.go Outdated Show resolved Hide resolved
Comment on lines 115 to 132
runIntervalHours, hasOk := d.GetOk("run_interval_hours")
if hasOk {
runIntervalHoursVal, _ := runIntervalHours.(int)
vars.RunIntervalHours = &runIntervalHoursVal

runStartsAt, hasOk := d.GetOk("run_starts_at")
if !hasOk {
return append(diags, diag.FromErr(fmt.Errorf("both run_interval_hours ad run_starts_at must be set to enable scheduling"))...)
}

runStartsAtVal, _ := runStartsAt.(string)
dt, err := time.Parse(reportRunStartsAtLayout, runStartsAtVal)
if err != nil {
return append(diags, diag.FromErr(fmt.Errorf("run_starts_at %s does not match layout %s", runStartsAtVal, reportRunStartsAtLayout))...)
}

vars.RunStartsAt = &dt
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be refactored into another func as the logic is repeated in update context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed: 38f2edc

internal/wiz/structs.go Show resolved Hide resolved
internal/wiz/structs.go Show resolved Hide resolved

func resourceWizReportGraphQuery() *schema.Resource {
return &schema.Resource{
Description: "TBD.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just spotted this TBD value, If you could please update then we can get this PR merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed: ec26333

Copy link
Collaborator

Choose a reason for hiding this comment

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

please run go generate ./... to update the docs and push the committed changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done: f3c5f9e

jschoombee
jschoombee previously approved these changes Dec 12, 2023
@jschoombee jschoombee merged commit 12b703d into AxtonGrams:main Dec 12, 2023
6 of 7 checks passed
@cvirtucio cvirtucio deleted the reports branch December 12, 2023 18:18
jschoombee added a commit that referenced this pull request Dec 13, 2023
* New Resource: wiz_report_graph_query
---------

Signed-off-by: Christopher Virtucio <[email protected]>
Signed-off-by: JP Schoombee <[email protected]>
Co-authored-by: JP Schoombee <[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.

New Resource: wiz_report_graph_query
2 participants