-
Notifications
You must be signed in to change notification settings - Fork 15
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
Implement service resource and data source in the framework provider #214
Implement service resource and data source in the framework provider #214
Conversation
2aff711
to
d8ea5e9
Compare
d8ea5e9
to
4b2f20f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review is in process, but I have commented tentatively. I completed the review and made some minor suggestions.
Since we merged another pull request, could you fix conflict?
internal/mackerel/service.go
Outdated
func ServiceNameValidator() validator.String { | ||
return stringvalidator.All( | ||
stringvalidator.LengthBetween(2, 63), | ||
stringvalidator.RegexMatches(regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9-_]+$`), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[may] Compiling regexp needs some initialize cost, so store the compiled expression as a global parameter is desirable.
var re = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9-_]+$`
func ServiceNameValidator() validator.String {
return stringvalidator.All(
stringvalidator.LengthBetween(2, 63),
stringvalidator.RegexMatches(re, "..."),
)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As ServiceNameValidator
is expected to be called constant times during a provider initialization, hoisting regex compilation may not be necessary. But it is some kind of best practices and there is no reason to deviate it: 91ffd1d
wantError: true, | ||
}, | ||
"invalid char": { | ||
val: types.StringValue("v('ω')v"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so cute lol
Validators: []validator.String{mackerel.ServiceNameValidator()}, | ||
}, | ||
"memo": schema.StringAttribute{ | ||
Computed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[memo] Former schema is here.
terraform-provider-mackerel/mackerel/data_source_mackerel_service.go
Lines 11 to 26 in 73005a2
func dataSourceMackerelService() *schema.Resource { | |
return &schema.Resource{ | |
ReadContext: dataSourceMackerelServiceRead, | |
Schema: map[string]*schema.Schema{ | |
"name": { | |
Type: schema.TypeString, | |
Required: true, | |
}, | |
"memo": { | |
Type: schema.TypeString, | |
Computed: true, | |
}, | |
}, | |
} | |
} |
Why memo has been treated as computed attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Computed attributes are "output" from the provider.
We can use memo
without specifying the value explicitly (an example below), so memo
(and id
) is a computed attribute.
data "mackerel_service" "svc" {
name = "service"
}
resource "some_resource" "something" {
some_value = data.mackerel_service.svc.memo
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id
may be used "input" value in the future (then name
and memo
are "output"). But it is out of scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood that the computed attribute would be unnecessary if the datasource could take a memo as input. Thank you for the information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
Output from acceptance testing: