feat(datawarehouse): add support for deployments, databases and users #3421
Conversation
jremy42
changed the title
jremy42
force-pushed
the
feat/datawarehouse_integration
branch
from
25d9e00 to
2a90b47
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3421 +/- ##
=========================================
- Coverage 1.66% 1.65% -0.02%
=========================================
Files 391 398 +7
Lines 43262 43702 +440
=========================================
+ Hits 720 722 +2
- Misses 42459 42897 +438
Partials 83 83 ☔ View full report in Codecov by Sentry. |
jremy42
force-pushed
the
feat/datawarehouse_integration
branch
8 times, most recently
from
d4d4d18 to
00245c3
Compare
jremy42
force-pushed
the
feat/datawarehouse_integration
branch
from
00245c3 to
abb244e
Compare
jremy42
changed the title
| tt := acctest.NewTestTools(t) | ||
| defer tt.Cleanup() | ||
|
|
||
| // Fetch a valid ClickHouse version to create a deployment |
There was a problem hiding this comment.
Could it be factored in a dedicated function?
| return datawarehouseapi.NewAPI(meta.ExtractScwClient(m)) | ||
| } | ||
|
|
||
| func expandStringList(list []any) []string { |
There was a problem hiding this comment.
Coud this function be migrated in the types package to be reused? Are we sure that there is no function in this package that already provide this feature?
|
|
||
| api := datawarehouse.NewAPI(tt.Meta) | ||
|
|
||
| versionsResp, err := api.ListVersions(&datawarehouseSDK.ListVersionsRequest{}, scw.WithAllPages()) |
There was a problem hiding this comment.
Could this be extracted in a function?
| } | ||
|
|
||
| func testSweepDatawarehouseDeployment(_ string) error { | ||
| return acctest.SweepRegions([]scw.Region{scw.RegionFrPar}, func(scwClient *scw.Client, region scw.Region) error { |
There was a problem hiding this comment.
Could it be possible to use datawarehouseSDK.Regions() here?
…icated helper function
… hardcoded region list
| func resourceDeploymentCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { | ||
| api := NewAPI(meta) | ||
|
|
||
| region := scw.Region(d.Get("region").(string)) |
There was a problem hiding this comment.
What if region is not specified? I think it would be helpful to have similar patterns between the different existing resource to manage locality.
| region := scw.Region(d.Get("region").(string)) | ||
| req := &datawarehouseapi.CreateDeploymentRequest{ | ||
| Region: region, | ||
| ProjectID: d.Get("project_id").(string), |
There was a problem hiding this comment.
I think it would be helpful to use types.ExpandStringPtr(d.Get("project_id"))
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| d.SetId(deployment.ID) |
There was a problem hiding this comment.
locality would be missing from this id :/
| return diag.FromErr(err) | ||
| } | ||
|
|
||
| _ = d.Set("region", string(region)) |
There was a problem hiding this comment.
Could you use the region coming from the deployment variable?
|
|
||
| // Helpers | ||
|
|
||
| // isDeploymentPresent is now defined in helpers_test.go |
| // Common helper functions shared across all datawarehouse tests | ||
|
|
||
| // fetchLatestClickHouseVersion returns the latest available ClickHouse version for testing purposes | ||
| func fetchLatestClickHouseVersion(t *testing.T, tt *acctest.TestTools) string { |
There was a problem hiding this comment.
t is already defined in tt :/ please simplify this function
jremy42
force-pushed
the
feat/datawarehouse_integration
branch
from
8ff6632 to
90e7b99
Compare
jremy42
force-pushed
the
feat/datawarehouse_integration
branch
2 times, most recently
from
ccd077f to
4ed5e39
Compare
jremy42
force-pushed
the
feat/datawarehouse_integration
branch
from
4ed5e39 to
89c284d
Compare
close #3102