fix assets_url attribute typo #448
Conversation
ghost
added
size/XS
dependencies
Type: Documentation
|
The CI failure looks related to a previously merged PR. /cc @anGie44 in case this looks familiar ☝️ |
|
@jcudit No, that CI failure was from me copying code without understanding it to try to make a deprecation message work. |
| @@ -147,7 +151,8 @@ func dataSourceGithubReleaseRead(d *schema.ResourceData, meta interface{}) error | |||
| d.Set("published_at", release.GetPublishedAt()) | |||
| d.Set("url", release.GetURL()) | |||
| d.Set("html_url", release.GetHTMLURL()) | |||
| d.Set("asserts_url", release.GetAssetsURL()) | |||
| d.Set("asserts_url", release.GetAssertsURL()) | |||
There was a problem hiding this comment.
hi @smiller171, thank you for catching this! I'd recommend leaving this as d.Set("asserts_url", release.GetAssetsURL()) so existing users/configs can still access this value with the method available in the Github Go SDK. If we go in the direction of Deprecating the asserts_url attribute, within the "assets_url" block, after L87, you can add the key/value pair: Deprecated: "Use assets_url instead",. please refer to these docs for additional examples and recommendations: https://www.terraform.io/docs/extend/best-practices/deprecations.html#renaming-a-computed-attribute
There was a problem hiding this comment.
Thanks for the link. I hope to have time to fix this soon
| @@ -11332,6 +11332,16 @@ func (r *RepositoryPermissionLevel) GetUser() *User { | |||
| return r.User | |||
| } | |||
|
|
|||
| // GetAssertsURL returns the AssetsURL field if it's non-nil, zero value otherwise. Created to generate a deprecation warning | |||
There was a problem hiding this comment.
this custom method can be safely omitted here as we can reuse GetAssetsURL() to populate the misspelled attribute
There was a problem hiding this comment.
My intent with the extra method was to have a place to handle the deprecation message. I haven't made the time to go through the deprecation docs you linked, but hopefully I'll get to that soon
|
@smiller171 are you around to take care of the conflict here? 🤔 seems like we can remove the edit to |
smiller171
force-pushed
the
fix-assets-url
branch
from
7442f21
to
eecafea
Compare
|
@jcudit I've rebased and reverted the change to Also it looks like a bunch of tests are failing now... |
|
Ah, didn't realize there was more to be done here. We can circle back on this effort another day. |
kfcampbell
added
Type: Maintenance
|
👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the |
|
👋 Hey Friends, this pull request has been automatically marked as |
I noticed that the attribute
assets_urlwas typod to beasserts_url. This PR is my attempt to fix that mistake without breaking anyone's existing Terraform code.I'm not sure I've handled the deprecation message properly, but I'm happy to make any adjustments needed.I definitely didn't do the deprecation message right and have removed it to allow the tests to pass. Any guidance on how to implement the deprecation message would be appreciated.