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

Remove Removed and prune related code #414

Merged
merged 1 commit into from
Apr 29, 2020
Merged

Remove Removed and prune related code #414

merged 1 commit into from
Apr 29, 2020

Conversation

paultyng
Copy link
Contributor

No description provided.

@paultyng paultyng added the breaking-change Implementation which breaks compatibility or other promises label Apr 28, 2020
@paultyng paultyng added this to the v2.0.0 milestone Apr 28, 2020
appilon
appilon reviewed Apr 28, 2020
View reviewed changes
Copy link
Contributor

@appilon appilon left a comment

Choose a reason for hiding this comment

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

Does Removed have any affect on state migrations?

helper/resource/testing_new_import_state.go
@@ -94,23 +92,6 @@ func testStepNewImportState(t testing.T, c TestCase, wd *tftest.WorkingDir, step
r.Primary.ID)
}

// We'll try our best to find the schema for this resource type
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure on this deletion, this was copy pasted directly from the old implementation of the acctest framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can let @kmoe chime in on this, but I think it was just to avoid the diff mismatch in import tests.

@paultyng
Copy link
Contributor Author

@appilon not as far as I can tell. I simulated manually adding an unrecognized field to state, and its ignored silently. So I don't think it serves any purpose.

@paultyng paultyng linked an issue Apr 28, 2020 that may be closed by this pull request
Drop "Removed" functionality from schema.Schema #320
Closed
paddycarver
paddycarver approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

I'm not an expert, so I'd advocate waiting for @appilon and @kmoe to weigh in, but I see nothing egregious.

@appilon
Copy link
Contributor

appilon commented Apr 28, 2020

If we can quickly confirm import tests are unaffected I'm fine with merging.

@appilon
Copy link
Contributor

appilon commented Apr 28, 2020

Needs a rebase after recent PR being merged

@paultyng
Copy link
Contributor Author

Rebased

@paultyng
Copy link
Contributor Author

paultyng commented Apr 29, 2020 •
edited
Loading

Here is a test run with import tests referencing this commit, it has no removed attributes though: https://github.com/paultyng/terraform-provider-unifi/actions/runs/91531504

@paultyng paultyng merged commit 8bc16c3 into master Apr 29, 2020
@paultyng paultyng deleted the remove-removed branch April 29, 2020 20:51
bflad added a commit to bflad/terraform-plugin-sdk that referenced this pull request Apr 30, 2020
@bflad
helper/schema: Mark Schema.Removed, SchemasForFlatmapPath, and (Resou…
8e68c9b 
…rce).SchemasForFlatmapPath() as Deprecated

Reference: hashicorp#414
Reference: https://github.com/golang/go/wiki/Deprecated

The `Removed` field and associated handling has been removed in version 2.
This was referenced Apr 30, 2020
Merged
Open
paultyng pushed a commit that referenced this pull request Apr 30, 2020
@bflad @paultyng
helper/schema: Mark Schema.Removed, SchemasForFlatmapPath, and (Resou…
6b372cb 
…rce).SchemasForFlatmapPath() as Deprecated

Reference: #414
Reference: https://github.com/golang/go/wiki/Deprecated

The `Removed` field and associated handling has been removed in version 2.
bflad added a commit to hashicorp/terraform-website that referenced this pull request Apr 30, 2020
@bflad
extend/best-practices/deprecations: Adjust process to exclude schema …
0963e28 
…attribute Removed usage

Reference: hashicorp/terraform-plugin-sdk#414

It will be removed in version 2 of the SDK, so we should no longer encourage its usage as a best practice.
@bflad bflad mentioned this pull request Apr 30, 2020
Merged
@bflad
Copy link
Contributor

bflad commented Apr 30, 2020

Fan out relating to this:

@paultyng
Copy link
Contributor Author

Thanks @bflad!

@breathingdust breathingdust mentioned this pull request May 19, 2020
Closed
@ghost
Copy link

ghost commented May 30, 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 May 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@appilon appilon appilon approved these changes

@paddycarver paddycarver paddycarver approved these changes

@kmoe kmoe Awaiting requested review from kmoe

Assignees
No one assigned
Labels
breaking-change Implementation which breaks compatibility or other promises
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.

Drop "Removed" functionality from schema.Schema
4 participants
@paultyng @appilon @bflad @paddycarver