Extend resource configuration API to include resource metadata#19
Conversation
594fdae to
d5053ea
Compare
There was a problem hiding this comment.
If we're planning on keeping this as single file, would it make sense to use go:embed similar to schema so we don't need to worry about reading files in Upjet?
There was a problem hiding this comment.
Looks like that if we use go:embed, we don't need to add RootDir and ProviderMetadataPath fields to config.Provider, which is preferable.
There was a problem hiding this comment.
I think at some point it's better to split the monolithic provider-metadata.yaml file. My proposal would be to do the per-resource splitting in further rounds, but we may also do so as part of the ongoing https://github.com/upbound/official-providers/issues/19 issue.
There was a problem hiding this comment.
Honestly, I can be convinced either way regarding single or multiple files. But if we do end up going with a single file, I'd prefer to go:embed it so that we do not deal with file reading at all in Upjet.
There was a problem hiding this comment.
Done. We now go:embed the monolithic provider metadata file. We may later split the provider metadata per resource in the future as discussed.
|
Thanks @muvaf for the comments! |
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
- Rename Resource.TitleName as Title - Add field documentation for Resource Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
| ProviderMetadataPath string | ||
| // ProviderMetadata is the scraped provider metadata | ||
| // from the corresponding Terraform registry. | ||
| ProviderMetadata string |
There was a problem hiding this comment.
Since we're using this only in NewProvider, would it make sense to not expose it in Provider struct?
| "forProvider": exampleParams, | ||
| "providerConfigRef": map[string]interface{}{ | ||
| "name": "example", | ||
| "name": "default", |
There was a problem hiding this comment.
nit: we can omit providerConfigRef altogether since it defaults to default.
| // NewProviderMetadataFromFile loads metadata from the specified YAML-formatted document | ||
| func NewProviderMetadataFromFile(providerMetadata string) (*ProviderMetadata, error) { | ||
| metadata := &ProviderMetadata{} | ||
| if err := yaml.Unmarshal([]byte(providerMetadata), metadata); err != nil { |
There was a problem hiding this comment.
Since we're converting to []byte anyway, we might consider accepting providerMetadata as []byte in NewProvider. go:embed supports it.
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Description of your changes
Fixes #6
This PR extends the existing upjet resource configuration API to allow configuration & overrides related to the resource metadata (probably scraped from Terraform registry). Some possible configuration scenarios covered as of now:
azurerm_key_vault_access_policy.example-user'skey_permissionsattribute.regionattribute for all/select resources inprovider-awslocationreferences inprovider-azureI have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
A resource specific metadata override example:
Or, as expected, a provider-wide metadata configuration can be performed as follows (in for example,
config/provider.go):An example manifest generated using these configuration overrides on top of scraped metadata:
To configure a unique name to be generated by the test runner, one may use:
The generated example manifest now contains:
which is to be substituted by the test runner at runtime with a random RFC1123 subdomain string.