refactor(albwaf): address open issues - #1667
Conversation
…hanges to other albwaf resources Relates to STACKITTPR-792
b54fa01 to
e3d94e9
Compare
cgoetz-inovex
left a comment
cgoetz-inovex
left a comment
There was a problem hiding this comment.
issue mentions:
- Place Min over Max test
probably missing from this PR
| if !req.State.Raw.IsNull() { | ||
| var stateModel Model | ||
| resp.Diagnostics.Append(req.State.Get(ctx, &stateModel)...) | ||
| if !resp.Diagnostics.HasError() { | ||
| utils.WarnIfNameChanges(stateModel.Name, planModel.Name, "Custom Rule Group", &resp.Diagnostics) | ||
| } | ||
| } | ||
|
|
||
| resp.Diagnostics.Append(resp.Plan.Set(ctx, planModel)...) | ||
| if resp.Diagnostics.HasError() { | ||
| return | ||
| } |
There was a problem hiding this comment.
Not sure if it matters, just some oddity
There's no early return here if line 355 creates an error:
resp.Diagnostics.Append(req.State.Get(ctx, &stateModel)...)when this happes we'd still call:
resp.Diagnostics.Append(resp.Plan.Set(ctx, planModel)...)Also the last conditional is unnecessary, but it looks like we have this in multiple places.
There was a problem hiding this comment.
From my understanding we're appending a list of errors here, so there is no need to necessarily return early here. I'm not sure if that's how it really works but if we skip the early return here, we are able to report the warning and any issues from parsing the planModel in one terraform apply command. If we instead return early, multiple "round trips" might be needed for that, no?
| ) | ||
| } | ||
| if !resp.Diagnostics.HasError() { | ||
| utils.WarnIfNameChanges(stateModel.Name, planModel.Name, "Managed Rule Set", &resp.Diagnostics) |
There was a problem hiding this comment.
same conditional logic as one file above
| albwafUtils.WarnIfNameChanges(stateModel.Name, planModel.Name, "WAF Configuration", &resp.Diagnostics) | ||
| } | ||
| } | ||
|
|
| resp, err := client.DefaultAPI.ListWAF(ctx, testutil.ProjectId, testutil.Region).Execute() | ||
| if err != nil { | ||
| return fmt.Errorf("getting resp: %w", err) | ||
| } | ||
|
|
||
| for _, item := range resp.Items { | ||
| if utils.Contains(wafConfigurationToDestroy, item.GetName()) { |
There was a problem hiding this comment.
https://docs.api.stackit.cloud/documentation/alb-waf/version/v1#tag/WAF/operation/WAFService_ListWAF
Looks like ListWAF uses pagination, docs don't mention a default pageSize. Is it enough to return all WAFs created in our acceptance tests?
| // waf configuration transform id: "[projectId],[region],[name]" | ||
| name := strings.Split(rs.Primary.ID, core.Separator)[2] |
There was a problem hiding this comment.
Sometimes I get an index out of bound panic on lines like these when running acceptance tests because rs.Primary.ID == "".
Id add an if here to prevent dangling resources.
2 participants
Description
relates to STACKITTPR-792
Checklist
make fmtexamples/directory)make generate-docs(will be checked by CI)make test(will be checked by CI)make lint(will be checked by CI)