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

fix(storagedirective): fix storage directive validator #538

Merged
merged 2 commits into from
Aug 7, 2024
Merged

fix(storagedirective): fix storage directive validator #538

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8c1beae
fix(storagedirective): fix storage directive validator
anvial Aug 1, 2024
0c493de
chore: updated comment for validator
anvial Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions internal/provider/validator_storagedirective.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"fmt"

"github.com/hashicorp/terraform-plugin-framework/schema/validator"
"github.com/juju/juju/storage"
jujustorage "github.com/juju/juju/storage"
)

type stringIsStorageDirectiveValidator struct{}
Expand All @@ -30,14 +30,21 @@ func (v stringIsStorageDirectiveValidator) ValidateMap(ctx context.Context, req
return
}

// If the value of any element is unknown or null, there is nothing to validate.
for _, element := range req.ConfigValue.Elements() {
if element.IsUnknown() || element.IsNull() {
return
Copy link
Member

Choose a reason for hiding this comment

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

thought: Should this be continue instead? What if there is more than one storage directive provided?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we just need to stop validating if we have at least one unknown among variables.

I've updated QA to touch case with multiple vars and and storage directives.
It looks like it's working.

Copy link
Member

Choose a reason for hiding this comment

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

todo: update the comment at line 33 to describe what is going on so we know if it breaks again. Below describes what I found and why this works.

It's only working because the method is called twice by terraform. I added some logging to see what is going on

Warning: CALLED ValidateMap "{\"block\":<unknown>,\"files\":\"73heather\"}"
Warning: CALLED ValidateMap "{\"block\":\"101M\",\"files\":\"73heather\"}"

The second call has the values filled in. I have no idea why terraform is doing this. If the behavior changes, we won't validate all the values.

}
}

var storageDirectives map[string]string
resp.Diagnostics.Append(req.ConfigValue.ElementsAs(ctx, &storageDirectives, false)...)
if resp.Diagnostics.HasError() {
return
}

for label, directive := range storageDirectives {
_, err := storage.ParseConstraints(directive)
_, err := jujustorage.ParseConstraints(directive)
if err != nil {
resp.Diagnostics.AddAttributeError(
req.Path,
Expand Down
Loading