-
Notifications
You must be signed in to change notification settings - Fork 13
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
ROX-21070: Add fleetshard addon installation status to private api #1503
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
a52b417
to
57b97b7
Compare
57b97b7
to
193174d
Compare
193174d
to
7535806
Compare
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, thanks for the cleanups as well
@@ -61,7 +59,7 @@ func (d *dataPlaneClusterService) GetDataPlaneClusterConfig(ctx context.Context, | |||
} | |||
|
|||
// UpdateDataPlaneClusterStatus ... | |||
func (d *dataPlaneClusterService) UpdateDataPlaneClusterStatus(ctx context.Context, clusterID string, status *dbapi.DataPlaneClusterStatus) *errors.ServiceError { | |||
func (d *dataPlaneClusterService) UpdateDataPlaneClusterStatus(clusterID string, status dbapi.DataPlaneClusterStatus) *errors.ServiceError { |
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 was surprised to see that ctx could just be removed here - digging down it looks like the clusterService in clusters.go doesn't use gorm's support for context (https://gorm.io/docs/context.html), so piping it down doesn't make sense. But do you know, is there a reason why there's no use of the context down at the database level? Is that best practice to not try to interrupt database calls with context / maybe there's some bad interaction between them?
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.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ebensh, kovayur The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
Install addon from Fleet Manager: Part 1.
This change adds a new field called
Addons
to theCluster
object to track the actual state of addon installations on the data plane.This change includes:
Checklist (Definition of Done)
Test manual
ROX-12345: ...
Test manual
Integration + E2E tests