-
Notifications
You must be signed in to change notification settings - Fork 1
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
Topograph model import for use with toposim #15
Conversation
…t models Signed-off-by: Henry Haase <[email protected]>
Signed-off-by: Henry Haase <[email protected]>
…el file Signed-off-by: Henry Haase <[email protected]>
…osim Signed-off-by: Henry Haase <[email protected]>
Signed-off-by: Henry Haase <[email protected]>
Signed-off-by: Henry Haase <[email protected]>
Signed-off-by: Henry Haase <[email protected]>
Signed-off-by: Dmitry Shmulevich <[email protected]>
…ameter. Signed-off-by: Henry Haase <[email protected]>
Signed-off-by: Henry Haase <[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.
We need to start putting more comments on public funcs. One small nit.
} else { | ||
model, err := models.NewModelFromFile(modelPath) | ||
if err != nil { | ||
return nil, err |
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.
nit: You unless properly wrapped by models.NewModelFromFile
, you'll want to wrap this error with
fmt.Errorf("could not read model file: %w", err)
If models.NewModelFromFile(modelPath)
DOES wrap the error, I would just add a comment
return nil, err // Wrapped by models.NewModelFromFile
This MR adds functionality for topograph to import a cluster model from a file to be use with toposim.