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

Update to tfplugin 5.2, map new protocol fields (v2) #349

Merged
merged 6 commits into from
Mar 18, 2020
Merged

Update to tfplugin 5.2, map new protocol fields (v2) #349

merged 6 commits into from
Mar 18, 2020

Conversation

paultyng
Copy link
Contributor

No description provided.

@paultyng paultyng changed the title Update to tfplugin 5.2 Update to tfplugin 5.2, map new protocol fields Mar 10, 2020
@paultyng paultyng linked an issue Mar 10, 2020 that may be closed by this pull request
Propagate Description, Kind, and Deprecated through new protocol #346
Closed
@paultyng paultyng requested a review from appilon March 11, 2020 00:45
appilon
appilon approved these changes Mar 11, 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.

Pretty much approved, super small nitpicks/curiosities

helper/schema/core_schema.go Show resolved Hide resolved
helper/schema/core_schema.go Show resolved Hide resolved
internal/plugin/convert/schema.go Show resolved Hide resolved
internal/plugin/convert/schema.go Show resolved Hide resolved
@paultyng paultyng marked this pull request as ready for review March 11, 2020 13:18
paddycarver
paddycarver suggested changes Mar 11, 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.

Looks like version2 got rebased, so this PR needs to rebase on version2.

@paultyng
Copy link
Contributor Author

rebased

@paultyng paultyng mentioned this pull request Mar 12, 2020
Merged
@paultyng paultyng changed the title Update to tfplugin 5.2, map new protocol fields Update to tfplugin 5.2, map new protocol fields (v2) Mar 12, 2020
paddycarver
paddycarver reviewed Mar 12, 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 seeing any new tests added for this code. I may have missed them (a lot of test files changed), but if not, is there any way for us to test this new functionality/

helper/schema/resource.go Show resolved Hide resolved
internal/plugin/convert/schema.go Show resolved Hide resolved
@paultyng
Map new proto fields
6d681c7 
Borrows some core code from 23acb02cb846fd002e7a0b52904ee88019b33211
@paultyng
Copy link
Contributor Author

@paddycarver added some test coverage for the global func at attribute level, there isn't any existing coverage at the resource level, but I can add it if you think its warranted.

paddycarver
paddycarver approved these changes Mar 18, 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.

Requested two log statements that you may have missed comments on a week ago. Not a big deal. Feel free to push them and merge, or merge without them and we'll address them if it ever comes up. I just see potential for bugs that we could make debugging easier, but it's not really a blocker.

internal/plugin/convert/schema.go Show resolved Hide resolved
internal/plugin/convert/schema.go Show resolved Hide resolved
@paultyng paultyng merged commit 35bf8ad into version2 Mar 18, 2020
@paultyng paultyng deleted the tfplugin5.2 branch March 18, 2020 14:26
@ghost
Copy link

ghost commented Apr 18, 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 18, 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

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

Successfully merging this pull request may close these issues.

Propagate Description, Kind, and Deprecated through new protocol
3 participants
@paultyng @paddycarver @appilon