Skip to content
This repository has been archived by the owner on Jul 6, 2022. It is now read-only.

Enhance plan schemas #665

Merged
merged 1 commit into from Jan 21, 2019

Conversation

piotrmiskiewicz
Copy link
Contributor

This PR enhance plan schemas by adding titles to enum values.
Related to #651

@msftclas
Copy link

msftclas commented Jan 16, 2019

CLA assistant check
All CLA requirements met.

@piotrmiskiewicz
Copy link
Contributor Author

The cosmos DB (Mongo) provides a schema with 'readRegions':

"readRegions": {
  "type": "array",
  "title": "Read Regions",
  "description": "Read regions to be created, your data will be synchronized across these regions."
                                      },

The items field is missing. It should looks like:

"allowedIPRanges": {
   "type": "array",
   "title": "OneOf IP ranges",
   "description": "Values to include in IP Filter. Can be an IP Address or CIDR range.",
   "items": {
   "type": "string",
   "description": "Must be a valid IP address or CIDR"
    }
 }

I'd like to add missing items, but I don't know what should be added there.

Copy link

@polskikiel polskikiel left a comment

Choose a reason for hiding this comment

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

You can also use the schemas.EnabledDisabledValues() in the following files:

  • pkg/services/rediscache/plan_schemas.go
  • pkg/services/storage/plan_schemas.go

@@ -82,7 +82,11 @@ func generateTopicBindingParamsSchema() service.InputParametersSchema {
Description: "Specifies whether to create a subscription in the topic." +
" Valid values are [\"yes\", \"no\"]. ",
AllowedValues: []string{"yes", "no"},

Choose a reason for hiding this comment

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

You can delete AllowedValues field and adjust the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -112,6 +112,7 @@ type StringPropertySchema struct {
AllowedPattern string `json:"pattern,omitempty"` // nolint: lll
CustomPropertyValidator CustomStringPropertyValidator `json:"-"`
DefaultValue string `json:"default,omitempty"` // nolint: lll
OneOf []EnumValue `json:"oneOf,omitempty"` //nolint: lll

Choose a reason for hiding this comment

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

Please add space to the //nolint comment to align it to the other occurences.

@zhongyi-zhang
Copy link
Contributor

Are the read regions the same set to locations @norshtein?

@norshtein
Copy link
Member

No, they are not the same. Allowed values of Cosmos read region can be found here. It is slightly different from location values.

@piotrmiskiewicz
Copy link
Contributor Author

I've added read locations to the Cosmos DB Mongo schema

@norshtein
Copy link
Member

I prefer to keep previous allowedValues for some time. Although it seems to be redundant, it can provide backward compatibility. Could you keep allowedValues unchanged?

@zhongyi-zhang
Copy link
Contributor

I think we don't need to consider backward compatibility in this case. The parsers which leverage React or Angular won't be impacted. On the contrary, with both AllowedValues and OneOf, I am not sure whether they are impacted. And, With the announcement in release note, if the change really breaks some parsers, the user still can wait for the fix in the parser side. Make sense?

@piotrmiskiewicz
Copy link
Contributor Author

React jsonschema (https://github.com/mozilla-services/react-jsonschema-form) does not work with both. The 'oneOf' field works very nice

@zhongyi-zhang
Copy link
Contributor

Oh, good to know. Then the old should be removed for sure. @piotrmiskiewicz thanks a lot for nice contribution! I’ll merge it as the CI also passed.

Copy link
Contributor

@zhongyi-zhang zhongyi-zhang left a comment

Choose a reason for hiding this comment

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

LGTM.

@zhongyi-zhang zhongyi-zhang merged commit 605d2dd into Azure:master Jan 21, 2019
@piotrmiskiewicz
Copy link
Contributor Author

piotrmiskiewicz commented Jan 22, 2019

When do you plan to do a new release?

@zhongyi-zhang
Copy link
Contributor

Soon. We are going to release after passing the release pipeline and a quick e2e test :).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants