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

Jump to bottom

Export resource deprecation. #18286

Merged
merged 2 commits into from
Jun 20, 2018
Merged

Export resource deprecation. #18286

merged 2 commits into from
Jun 20, 2018

Conversation

paddycarver
Copy link
Contributor

We already had the functionality to make resources deprecated, which was
used when migrating resources to data sources, but the functionality was
unexported, so only the schema package could do it. Now it's exported,
meaning providers can mark entire resources as deprecated. I also added
a test in hopefully-the-right place?

@paddycarver
Export resource deprecation.
0dd2408 
We already had the functionality to make resources deprecated, which was
used when migrating resources to data sources, but the functionality was
unexported, so only the schema package could do it. Now it's exported,
meaning providers can mark entire resources as deprecated. I also added
a test in hopefully-the-right place?
@paddycarver paddycarver mentioned this pull request Jun 19, 2018
Merged
@apparentlymart apparentlymart requested a review from a team June 20, 2018 17:00
apparentlymart
apparentlymart reviewed Jun 20, 2018
View reviewed changes
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'd also like to give our friends who maintain the provider SDK an opportunity to weigh in here in case they see any conflicts with plans I don't know about.

@appilon
Copy link
Contributor

appilon commented Jun 20, 2018

Whoa this is cool that this exists! We actually had an asana task for this, it had some early api designs but if this already exists and has been used then maybe we just merge and mark that task as complete?

@paddycarver
Copy link
Contributor Author

I'm game! If we're going to abandon this for the Asana task, however, I'd like to know that ~soon, as we're working against a deadline here to help make sure hashicorp/terraform-provider-consul#49 can meet its deadlines. I undertook this PR on the understanding that this is something we could probably get merged this week, but if it gets more involved or the timeline starts looking shaky, I just want to change how we're handling hashicorp/terraform-provider-consul#49 :)

@apparentlymart
Copy link
Contributor

I'd suggest that if there aren't any known conflicts between this and our future plans that we move ahead with it for now to get the Consul PR unblocked but then use our internal tracking item to mean "think about a longer-term solution here", which might eventually end up just being closed as this being good enough but we'll have an opportunity to make a better plan then if we find one.

This mechanism was originally introduced back in 0.7 to allow us to deprecate resource "template_file" in favor of data "template_file" (for example), but at the time I'd cautiously left it unexported in case we wanted to remove it once the deprecation cycle for those was completed. However, since we now have Deprecated on resource attributes exported, this seems nice to do for consistency.

appilon
appilon requested changes Jun 20, 2018
View reviewed changes
helper/schema/data_source_resource_shim.go Outdated
@@ -32,7 +32,7 @@ func DataSourceResourceShim(name string, dataSource *Resource) *Resource {

// FIXME: Link to some further docs either on the website or in the
// changelog, once such a thing exists.
dataSource.deprecationMessage = fmt.Sprintf(
dataSource.Deprecated = fmt.Sprintf(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think some of this feedback is expressed in Slack, but I think DeprecationMessage is the preferred var name. Other than that I see no reason to block this, we can build off it or keep it as our "Resource Deprecation" mechanism!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@paddycarver
Deprecated -> DeprecationMessage.
b7d19e4 
At the Enablement team's request, change from using `Deprecated` to
`DeprecationMessage`, as it's a string value, not a boolean.
@paddycarver
Copy link
Contributor Author

Build failure is unrelated to these changes--looks like HEAD on master is broken. Merging!

@paddycarver paddycarver reopened this Jun 20, 2018
@paddycarver paddycarver merged commit d35fe51 into master Jun 20, 2018
@radeksimko radeksimko deleted the paddy_deprecate_resource branch June 21, 2018 06:59
This was referenced Jul 13, 2018
Closed
Merged
@ghost
Copy link

ghost commented Apr 3, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apparentlymart apparentlymart apparentlymart left review comments

@appilon appilon appilon approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paddycarver @appilon @apparentlymart