-
Notifications
You must be signed in to change notification settings - Fork 386
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 support for Databricks Repos #771
Conversation
Codecov Report
@@ Coverage Diff @@
## master #771 +/- ##
==========================================
+ Coverage 83.42% 83.54% +0.12%
==========================================
Files 91 92 +1
Lines 8227 8364 +137
==========================================
+ Hits 6863 6988 +125
- Misses 862 869 +7
- Partials 502 507 +5
|
this would be used when checkout the git repo into "custom" folder inside user's Repos location
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.
Please adjust repos resource to reflect_resource
|
||
func ResourceRepo() *schema.Resource { | ||
s := common.StructToSchema(ReposInformation{}, func(s map[string]*schema.Schema) map[string]*schema.Schema { | ||
s["url"].ForceNew = true |
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.
i wonder if we should add force_new
to reflect_resource
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.
I thought about that, just didn't want to pollute the PR for specific functionality
} | ||
// TODO: update may change ONE OF (url AND provider (optional)), (path), or (branch OR tag). | ||
// for URL/provider force re-create as there are limits on what could be done for changing URL/provider | ||
if path, ok := r["path"]; ok { |
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.
i think the code would be cleaner if this method accepts this struct
type updateRequest struct {
Branch string `json:"branch,omitempty"`
Tag. string `json:"tag,omitempty"`
}
because it's going to be easier to use common.DataToStructPointer
& common.StructToData
and there'll be less errors to handle.
and then the schema might be composed of calling StructToSchema twice?.. usage of reflection simplifies resource maintenance, because we have to deal just with structs...
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.
Addition of path, and necessity of handling it differently makes things harder - setting branch/tag here is different from Update method. Although maybe we’ll need th handle it later, when it’s implemented
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.
Update of path-only will come in the near future, so it will be easier to change code with current approach
return err | ||
} | ||
d.SetId(resp.RepoID()) | ||
branch := d.Get("branch").(string) |
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.
technically, you can re-use the update method here, 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.
update logic is different here compared to Update method - I don't want to re-checkout branch if it's already checked out. Correct way would be if the Create API had the branch
and tag
parameters, but it's not implemented there - that's why two calls. On other side, I can specify branch
parameter set to the default branch of repository, that's why we have branch != resp.Branch
condition
This PR adds
databricks_repo
resource to manage Databricks Repos