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

Add description to max_clusters_per_user in policies schema. Re-format docs. #1959

Merged
merged 5 commits into from
Jan 27, 2023

Conversation

iandexter
Copy link
Contributor

No description provided.

@iandexter iandexter requested review from nfx and alexott January 26, 2023 10:49
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2023

Codecov Report

Merging #1959 (26c0ece) into master (2086e9b) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1959      +/-   ##
==========================================
+ Coverage   90.24%   90.29%   +0.05%     
==========================================
  Files         146      146              
  Lines       11898    11906       +8     
==========================================
+ Hits        10737    10751      +14     
+ Misses        749      746       -3     
+ Partials      412      409       -3     
Impacted Files Coverage Δ
common/version.go 100.00% <ø> (ø)
policies/resource_cluster_policy.go 94.93% <100.00%> (+9.02%) ⬆️

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

exclude go.mod & go.sum from PR. We need to test that it allows 0 value (when it's omitted).

You can easily test by building it and specifying dev override like described here or here

@@ -99,7 +99,7 @@ The following arguments are required:

* `name` - (Required) Cluster policy name. This must be unique. Length must be between 1 and 100 characters.
* `definition` - (Required) Policy definition: JSON document expressed in [Databricks Policy Definition Language](https://docs.databricks.com/administration-guide/clusters/policies.html#cluster-policy-definition).
* `max_clusters_per_user` - (Optional, integer) Maximum number of clusters allowed per user. When omitted, there is no limit.
* `max_clusters_per_user` - (Optional, integer) Maximum number of clusters allowed per user. When omitted, there is no limit. If specified, value must be non-zero.
Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to say "greater than zero" :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@@ -125,6 +125,9 @@ func ResourceClusterPolicy() *schema.Resource {
"max_clusters_per_user": {
Type: schema.TypeInt,
Optional: true,
Description: "Max number of clusters per user that can be active\n" +
"using this policy. If not present, there is no max limit.",
ValidateFunc: validation.IntAtLeast(1),
Copy link
Contributor

Choose a reason for hiding this comment

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

does it work when omitted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've tested it with and without the parameter.

Copy link
Contributor

@alexott alexott left a comment

Choose a reason for hiding this comment

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

lgtm

@nfx nfx enabled auto-merge (squash) January 27, 2023 14:36
@nfx nfx merged commit 8660c0f into databricks:master Jan 27, 2023
@iandexter iandexter deleted the cluster_policy branch January 27, 2023 14:48
@nfx nfx mentioned this pull request Feb 3, 2023
michael-berk pushed a commit to michael-berk/terraform-provider-databricks that referenced this pull request Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants