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

Fixed value length check #2221

Merged
merged 6 commits into from
Nov 6, 2018
Merged

Fixed value length check #2221

merged 6 commits into from
Nov 6, 2018

Conversation

WodansSon
Copy link
Collaborator

No description provided.

Copy link

@JunyiYi JunyiYi left a comment

Choose a reason for hiding this comment

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

Of course.

@tombuildsstuff
Copy link
Contributor

hey @jeffreyCline

Thanks for this PR :)

Taking a quick look at the portal it appears the maximum supported value is 30 (and not 128):

screenshot 2018-11-02 at 23 54 12

As such could we invert this change (to fix the comments/error message/tests rather than the limit?) - but in general I'm 👍 with this change :)

Thanks!

@katbyte
Copy link
Collaborator

katbyte commented Nov 3, 2018

Might I suggest actually hitting the API? multiple times i've found the value in the portal to be vastly different then what the API will allow.

@metacpp
Copy link
Contributor

metacpp commented Nov 4, 2018

Taking a quick look at the portal it appears the maximum supported value is 30 (and not 128):

screenshot 2018-11-02 at 23 54 12

But its length definition is between 3 and 64 in API Swagger:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/databricks/resource-manager/Microsoft.Databricks/stable/2018-04-01/databricks.json#L684

@WodansSon
Copy link
Collaborator Author

@ghost ghost added size/S and removed size/XS labels Nov 5, 2018
@WodansSon WodansSon closed this Nov 5, 2018
@WodansSon WodansSon reopened this Nov 5, 2018
@metacpp
Copy link
Contributor

metacpp commented Nov 6, 2018

According to the azure-sdk-for-go they too have implemented the length as 3 to 64...

@jeffreyCline this is because of SDK was generated from API swagger:
https://github.com/Azure/azure-rest-api-specs/blob/master/specification/databricks/resource-manager/Microsoft.Databricks/stable/2018-04-01/databricks.json#L684

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Thanks for pushing those changes @jeffreyCline - this now LGTM 👍

@tombuildsstuff tombuildsstuff merged commit 7b65540 into master Nov 6, 2018
@tombuildsstuff tombuildsstuff deleted the u_databricks_workspace branch November 6, 2018 09:18
tombuildsstuff added a commit that referenced this pull request Nov 6, 2018
@ghost
Copy link

ghost commented Mar 6, 2019

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
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.

5 participants