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

Added support for iam_role_arn in databricks_instance_profile #1943

Merged
merged 12 commits into from
Jan 25, 2023

Conversation

Jorge-Rodriguez
Copy link
Contributor

This PR adds support to manage an instance profile's IAM role ARN when it does not match the instance profile name. This feature is needed when managing instance profiles for use with Databricks SQL serverless.

See Databricks REST API documentation for instance profiles

Fixes #1911
Fixes #1710

@Jorge-Rodriguez
Copy link
Contributor Author

Please note that this is my first stab at writing anything in go, so feel free to consider this PR just as a PoC for the intended support feature if there is anything fundamentally wrong otherwise.

@Jorge-Rodriguez
Copy link
Contributor Author

ping @alexott

aws/resource_instance_profile.go Outdated Show resolved Hide resolved
aws/resource_instance_profile.go Outdated Show resolved Hide resolved
@alexott
Copy link
Contributor

alexott commented Jan 20, 2023

Also, add documentation

@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2023

Codecov Report

Merging #1943 (7a725cb) into master (79c6338) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1943      +/-   ##
==========================================
+ Coverage   90.19%   90.29%   +0.09%     
==========================================
  Files         146      146              
  Lines       11843    11888      +45     
==========================================
+ Hits        10682    10734      +52     
+ Misses        748      742       -6     
+ Partials      413      412       -1     
Impacted Files Coverage Δ
aws/resource_group_instance_profile.go 100.00% <100.00%> (ø)
aws/resource_instance_profile.go 71.09% <100.00%> (+12.60%) ⬆️
aws/resource_user_instance_profile.go 100.00% <100.00%> (ø)
sql/api/query.go 75.40% <0.00%> (ø)
clusters/clusters_api.go 85.71% <0.00%> (ø)
sql/resource_sql_query.go 95.80% <0.00%> (+0.02%) ⬆️
exporter/importables.go 90.22% <0.00%> (+0.03%) ⬆️
libraries/libraries_api.go 94.90% <0.00%> (+0.03%) ⬆️
storage/aws_s3_mount.go 87.65% <0.00%> (+0.15%) ⬆️
sql/resource_sql_dashboard.go 88.67% <0.00%> (+0.44%) ⬆️
... and 3 more

aws/resource_instance_profile.go Outdated Show resolved Hide resolved
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. Let check with the PR runner

@Jorge-Rodriguez
Copy link
Contributor Author

lgtm. Let check with the PR runner

There seems to be a typo

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.

the diff coverage is only 53.33%.

Can you add test cases for ValidInstanceProfile function for following pieces that aren't covered:

  • if !ok {
  • if len(arnSections) != 6 {

and in ValidIamRole

  • if !ok {
  • if len(arnSections) != 6 {
  • if !strings.HasPrefix(arnSections[5], "role") {

And really, the ValidInstanceProfile and ValidIamRole are only different in the line

if !strings.HasPrefix(arnSections[5], "role")

But really you can create a function that will receive role or instance-profile as an argument and return result...

@Jorge-Rodriguez
Copy link
Contributor Author

Sure. That'll handle the DRY issue as well

@alexott
Copy link
Contributor

alexott commented Jan 24, 2023

One thing that is different between instance profile & role validation is that we're allowing empty role, while instance profile shouldn't allow it

@Jorge-Rodriguez
Copy link
Contributor Author

One thing that is different between instance profile & role validation is that we're allowing empty role, while instance profile shouldn't allow it

Oh, I got confused by the fact that InstanceProfileArn is marked as omitempty

In other matters, I don't seem to be able to write a test for if !ok {, I can't think of a configuration value that wouldn't get marshalled into a string without throwing a panic before even starting the test.

@alexott
Copy link
Contributor

alexott commented Jan 24, 2023

Hmmm, interesting - omitempty is wrong here - doc says that this field is required so omitempty should be removed

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 - please look & merge

@nfx nfx changed the title databricks_instance_profile: Add support for iam_role_arn parameter Added support for iam_role_arn attribute in databricks_instance_profile Jan 25, 2023
@nfx nfx changed the title Added support for iam_role_arn attribute in databricks_instance_profile Added support for iam_role_arn in databricks_instance_profile Jan 25, 2023
@nfx nfx merged commit c27ccd6 into databricks:master Jan 25, 2023
@nfx nfx mentioned this pull request Jan 25, 2023
michael-berk pushed a commit to michael-berk/terraform-provider-databricks that referenced this pull request Feb 15, 2023
…tabricks#1943)

* Add IamRoleArn to instance_profile resource

* Add tests

* Add handling for empty string role ARN

* Add documentation

* Modifications according to PR comments

* Fix typo

* Unify ARN validation functions

* Rename validation tests

* Do not allow empty instance profile ARNs

* Remove `omitempty` from required field

* Add default clause
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.

[FEATURE] New feature request : [FEATURE] Add iam_role_arn for databricks_group_instance_profile resource
4 participants