-
Notifications
You must be signed in to change notification settings - Fork 12
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
model: Switching from multi-source to single-source #142
Conversation
28106b9
to
7ed5d66
Compare
4bcd2bc
to
9750f4d
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.
Really impressive PR! I did not have any comments on the logic, added some comments that are opinions, some small things about comments, and one critical things about which organization Batman belongs to :)) Regarding the logs for 500, I added in the beginning but then we had the talk during the office hours - definitely middle-wear and not for this PR.
I would also say I think perhaps some refactoring can be done to have the before and after each blocks extracted to some utils file.
Anyway - great PR, there is nothing in my comments I would insist on, though I think they are potential improvements.
- if the source has no inventory yet, set the vCenterID and AssociatedAgentID to this source. | ||
*/ | ||
func (h *AgentServiceHandler) UpdateSourceInventory(ctx context.Context, request agentServer.UpdateSourceInventoryRequestObject) (agentServer.UpdateSourceInventoryResponseObject, error) { | ||
source, err := h.store.Source().Get(ctx, request.Id) |
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.
maybe we should add some logging before returning the errors here? for example we are returning UpdateSourceInventory400JSONResponse in two places, how will we know which one happened?
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 would add a wrapper around this handler (2 methods only) to log the request and the response.
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.
@machacekondra wdyt?
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.
Could we do it in follow-up PR? So we have this one merged?
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 of course
if err != nil && !errors.Is(err, store.ErrRecordNotFound) { | ||
return agentServer.UpdateAgentStatus400JSONResponse{}, nil | ||
return agentServer.UpdateAgentStatus500JSONResponse{}, nil |
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.
log?
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.
same as above
This commit changes the model from multi-source to single-source (i.e one source - one agent). Signed-off-by: Cosmin Tupangiu <[email protected]>
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! :)
This commit changes the model from multi-source to single-source (i.e one source - one agent).
The underlying assumption is that a source always has an agent even in the on-prem scenario.
Changes:
db: the relationship between source and agent is one2one.
api: agent api do not change only the main api changes to reflect the logic of the new model.
Singed-off-by: Cosmin Tupangiu [email protected]