Ensure empty required object properties are in requests#2856
Merged
Conversation
Add a single manual required container until we can derive from the spec.
We're interested in the request to make, when constructing the containers, rather than the response.
Contributor
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov Report
@@ Coverage Diff @@
## master #2856 +/- ##
==========================================
+ Coverage 58.28% 58.38% +0.10%
==========================================
Files 54 55 +1
Lines 10005 10056 +51
==========================================
+ Hits 5831 5871 +40
- Misses 3651 3660 +9
- Partials 523 525 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Make type public outside of the module
This fails without the required containers.
danielrbradley
changed the title
- Make: Skip expanding then passing the list of go packages. `./...` does this for us. - Use gotestfmt in CI/CD for better output. Still set same coverage flags as in make. - Combine provider and arm2pulumi prebuild steps to make CI prebuild easier. - Increase explicit `-parallel` from program test in CI to 16. Tests are not bound on local resources but rather on waiting for external services. - Add actionlint.yaml config to silence warnings about self-hosted runners.
thomas11
approved these changes
Oct 27, 2023
thomas11
left a comment
thomas11
left a comment
Contributor
There was a problem hiding this comment.
LGTM with two not strictly required suggestions.
- When merging containers, concatenate each entry so we can index it, then only add if we've not seen it before. - Add docs for the additional required containers.
Managed Environments take >8 minutes to create and destroy. Query Pack takes less than 2 minutes. - Rename the test to indicate its purpose. - Tested locally that reverting the provider fix causes the test to fail with "Value cannot be null.\r\nParameter name: 'properties' property must be specified"
danielrbradley
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
App.ManagedEnvironment: "Must containproperties" #2578 (comment))Fixes #2190
Fixes #2578
Based on a query of the generated metadata, this affects 337 resources (939 when considering multiple versions of the same resource):