fix: add or update fields in the existing environment or rulesets#665
fix: add or update fields in the existing environment or rulesets#665decyjphr merged 6 commits intogithub:main-enterprisefrom luvsaxena1:main-enterprise
Conversation
luvsaxena1
changed the title
|
LGTM, can we get this merged? |
|
@klutchell I have tested this issue on my local version of safe settings and its working as expected. |
luvsaxena1
changed the title
|
WDYT @decyjphr ? |
|
Reviewing |
|
This is tricky @luvsaxena1 did you happen to see PR #649 Environments tolerate concise configuration? It combines together other PRs you have commented in, and it also addresses the issue you recently raised in #660. I confirmed this by locally adding a new unit test for the scenario you outlined in 660. When I made the changes for #649, I was intentionally constraining the area of impact to environments in the hope that this would make the PR easier to review and accept. That being said, I also like that you have gone after the broader change in mergeDeep I've brought just the changes from this PR #665 locally, and run the updated unit tests for environments from PR #649. There is certainly an improvement that has come to environments from the mergeDeep changes. However there are still 5/12 unit tests which fail. 
I'll give this some further thought and update back here |
decyjphr
left a comment
decyjphr
left a comment
There was a problem hiding this comment.
I know the mergeDeep is not optimal and could be improved. So adding this extract bit to do handle nested objects when adding elements to additions is great. Thanks.
|
@luvsaxena1 do you want us to take a look at resolving the tests or you are looking at it already? Don't want to muddy the waters... |
|
Tested locally and the tests seem to pass. |
@decyjphr, I think that perhaps this question is for me to answer. If #649 is also approved and merged, then I anticipate that it will take care of resolving the tests. I'll confirm and follow up here. |
@decyjphr confirming that this PR and #649 are compatible. Combining the two still results in the expanded set of environments unit tests all passing.
|
|
Are we set to merge this next? Now that #649 has merged it would be nice to get the fixes to mergeDeep to catch changes to rulesets and nested objects in general. |
|
@decyjphr Can we merge this PR next ? As I said this also solve problem with nested objects of rulesets and other plugins. |
|
@decyjphr would it be too much trouble to drop an rc release with this change? ❤️ |
|
@klutchell I created 2.1.11-rc.2 hope that helps |
|
Thanks! |
…thub#665) * feat: initial commit * fix: merge deep code * fix: lint * fix: modification conditions * fix: simplify conditions --------- Co-authored-by: ls07667 <saxenaluv@johndeere.com>
4 participants
This is to fix issue issue. This could potentially fix adding nested object in the rulesets as well.