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.

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

helper/schema: Make nested Set(s) in List(s) work (2) #7485

Closed

Conversation

radeksimko
Copy link
Member

This is a second (patched) version of #7393 which had to be reverted because some acceptance tests were failing as part of the nightly run.

Related to #7253 & #7254
Supersedes/fixes #7268
Blocking #7395


The only difference from the original PR is the last commit (50fbe79).


I managed to reproduce schema diff edge cases from the failing tests in aws provider and fix those. I added 3 new unit tests to cover this new functionality.

I ran the 16 (previously failing) aws acceptance tests, all are now passing. I also ran first aws 141 of total 464 acceptance tests which all (eventually) passed.

I don't have a Fastly account so I would appreciate if someone could double check all acceptance tests are passing there too and generally run the whole shebang once again with this patch (possibly before merging).

@radeksimko radeksimko changed the title helper/schema: Make nested Set(s) in List(s) work (2) [WIP] helper/schema: Make nested Set(s) in List(s) work (2) Jul 7, 2016
@radeksimko radeksimko force-pushed the b-schema-set-in-list branch 2 times, most recently from 9fb0a4b to 240f5bd Compare July 7, 2016 10:38
@radeksimko radeksimko changed the title [WIP] helper/schema: Make nested Set(s) in List(s) work (2) helper/schema: Make nested Set(s) in List(s) work (2) Jul 7, 2016
@apeeters
Copy link

Some tests still seem to be failing, was this expected?

@radeksimko
Copy link
Member Author

Some tests still seem to be failing, was this expected?

Those are unrelated acceptance test failures.

@apeeters
Copy link

Then what is holding this PR back? I would love #7395 to be implemented soon :)

@radeksimko radeksimko force-pushed the b-schema-set-in-list branch from 240f5bd to a727c0c Compare July 29, 2016 18:39
@radeksimko
Copy link
Member Author

I have just rebased this which also restarted tests and should remove the relationship with failing Travis build w/ acceptance tests.

It is possible that we may delay merging this PR (i.e. #7395 too) until 0.7 is out - which should be very soon. 😉

@radeksimko radeksimko force-pushed the b-schema-set-in-list branch from a727c0c to 8811feb Compare August 18, 2016 12:10
@radeksimko radeksimko force-pushed the b-schema-set-in-list branch from 8811feb to 58e8910 Compare August 28, 2016 21:59
@apeeters
Copy link

Any updates on this now 0.7 is out? :)

Although DiffFieldReader was the one mostly responsible for a buggy behaviour
more tests were added throughout the debugging process most of which
would fail without the bugfix.

 - ResourceData
 - MultiLevelFieldReader
 - MapFieldReader
 - DiffFieldReader
@mitchellh
Copy link
Contributor

Hey @radeksimko, I'm interested in getting this into master but it appears to panic on tests when rebased now. Can you rebase this and take a look?

@catsby
Copy link
Contributor

catsby commented Jan 26, 2018

Hey @radeksimko is this a thing you plan to complete sometime, or can it be closed? 😄

@radeksimko
Copy link
Member Author

Let's close this for now.

@radeksimko radeksimko closed this Jan 31, 2018
@ghost
Copy link

ghost commented Apr 5, 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 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants