refactor: Extract common resource fields into a separate embedded struct #3571
Conversation
andrewnester
requested review from
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
bundle/config/resources/apps.go
Outdated
| ID string `json:"id,omitempty" bundle:"readonly"` | ||
| // We cannot embed BaseResource here because apps.App has its own Id field | ||
| // that conflicts with BaseResource.ID | ||
| ID string `json:"id,omitempty" bundle:"readonly"` |
There was a problem hiding this comment.
Question - where does this conflict manifests? Curious why it's a problem for 2 embedded structs but not a problem for outer + embeded.
There was a problem hiding this comment.
I suspect it's a precedence thing where the second embedded struct takes precedence over the first.
There was a problem hiding this comment.
in that case swapping order would allow to use embedded one here?
BTW URL also overlaps for Apps
There was a problem hiding this comment.
I think it's still the same problem with outer + embedded, it's just that linter does not catch it, I fixed it here to use BaseResource for apps and pipelines anyway
d584606
|
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
@andrewnester Any ideas if we could fix the cases where we can't embed it? It almost defies the purpose of having a single base struct to reuse, if we have to copy/paste fields to the cases where it doesn't work anyway.
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Changes
Extract common resource fields into a separate embedded struct
BaseResourceWhy
Based on feedback from #3448 (comment)
Tests
Existing tests pass