-
Notifications
You must be signed in to change notification settings - Fork 70
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
CLOUDP-70044: mongocli ops-manager|cloud-manager maintenanceWindow(s) create #428
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.
One small nit but not blocking
@@ -83,3 +88,14 @@ func (s *Store) MaintenanceWindow(projectID string) (*atlas.MaintenanceWindow, e | |||
return nil, fmt.Errorf("unsupported service: %s", s.service) | |||
} | |||
} | |||
|
|||
// CreateMaintenanceWindow encapsulates the logic to manage different cloud providers | |||
func (s *Store) CreateMaintenanceWindow(projectID string, maintenanceWindow *opsmngr.MaintenanceWindow) (*opsmngr.MaintenanceWindow, error) { |
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'm not sure what your plane is when we get to do get/update/rm for ops manager given their signature will probably be different so I'm not sure we should keep them in the same file
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 thinking about calling the interface like "OpsManagerMaintenanceWindowsUpdater". Would you rather have one file or two?
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.
if you're going to have OpsManagerMaintenanceWindowUpdater
, we should have OpsManagerMaintenanceWindowCreator
, I would also make the methods explicit
CreateOpsManagerMaintenanceWindow
and UpdateOpsManagerMaintenanceWindow
to avoid confusion.
Separate files is up to you so far I don't have strong opinion on it specially since files within the same package are just symbolic, my only comment is on the name of the interfaces and methods, again to avoid confusion
Feel free to address any of this in a separate PR
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
@@ -83,3 +88,14 @@ func (s *Store) MaintenanceWindow(projectID string) (*atlas.MaintenanceWindow, e | |||
return nil, fmt.Errorf("unsupported service: %s", s.service) | |||
} | |||
} | |||
|
|||
// CreateMaintenanceWindow encapsulates the logic to manage different cloud providers | |||
func (s *Store) CreateMaintenanceWindow(projectID string, maintenanceWindow *opsmngr.MaintenanceWindow) (*opsmngr.MaintenanceWindow, error) { |
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.
if you're going to have OpsManagerMaintenanceWindowUpdater
, we should have OpsManagerMaintenanceWindowCreator
, I would also make the methods explicit
CreateOpsManagerMaintenanceWindow
and UpdateOpsManagerMaintenanceWindow
to avoid confusion.
Separate files is up to you so far I don't have strong opinion on it specially since files within the same package are just symbolic, my only comment is on the name of the interfaces and methods, again to avoid confusion
Feel free to address any of this in a separate PR
Proposed changes
Jira ticket: CLOUDP-70044
Checklist
make fmt
and formatted my codeFurther comments