-
Notifications
You must be signed in to change notification settings - Fork 398
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
Allow custom tags on AWS customer managed VPC workspaces #3114
Allow custom tags on AWS customer managed VPC workspaces #3114
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good. Let's run integration tests
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3114 +/- ##
==========================================
+ Coverage 83.99% 84.30% +0.31%
==========================================
Files 155 162 +7
Lines 13669 14414 +745
==========================================
+ Hits 11481 12152 +671
- Misses 1538 1565 +27
- Partials 650 697 +47
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
||
// UpdateRunning will update running workspace with couple of possible fields | ||
func (a WorkspacesAPI) UpdateRunning(ws Workspace, timeout time.Duration) error { | ||
workspacesAPIPath := fmt.Sprintf("/accounts/%s/workspaces/%d", ws.AccountID, ws.WorkspaceID) | ||
request := map[string]string{} | ||
request := map[string]any{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
custom tags are map of string to string right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it's a map, but this change is required because we need to nest custom tags inside the map that will be sent to Workspace API
Changes
Reference:
https://docs.databricks.com/api/account/workspaces/create
https://docs.databricks.com/api/account/workspaces/update
This will also solve issue #2938
At my company we want to try keep as much in terraform code as possible. We have a temporarily call once workspace is created to update the tags for billing purposes but keeping in terraform code would be more suitable!
Tests
Manually tested with Company Databricks account and workspace on AWS environment. Note tested via GCP / Azure as currently do not have access.
make test
run locallydocs/
folderinternal/acceptance
This is my first attempted open source contribution. I am also learning golang and tried to attempt this as a real problem to solve in my workplace. So feel free to scrutinize fully.