-
Notifications
You must be signed in to change notification settings - Fork 23
[OCTRL-1012]: GetEnvironments is missing ODC devices info even with flag set #720
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
Conversation
core/protos/o2control.proto
Outdated
| message GetEnvironmentsRequest { | ||
| bool showAll = 1; | ||
| bool showTaskInfos = 2; | ||
| bool showIntegratedServices = 3; |
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 think the flag name is unclear. Even when it's false, we fill ingratedServicesData with the output of GetEnvironmentsShortData, while the flag name gives impression we will get nothing. Could we name it for example showDetailedIntegratedServices, or anything conveying this meaning? A short comment might also help.
The fact that showAll does not imply we show task infos is another issue, but it's something that was there before.
core/server.go
Outdated
|
|
||
| if request.GetShowIntegratedServices() { | ||
| e.IntegratedServicesData = integration.PluginsInstance().GetEnvironmentsData([]uid.ID{env.Id()})[env.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.
Is there a reason to put this code here instead of combining it with this call from above?
integratedServicesEnvsData := integration.PluginsInstance().GetEnvironmentsShortData(m.state.environments.Ids())
Depending on the flag, we could call either GetEnvironmentsShortData or GetEnvironmentsData instead of calling always the first, and optionally overwriting it with the 2nd.
core/server.go
Outdated
| integratedServicesEnvsData = integration.PluginsInstance().GetEnvironmentsShortData(m.state.environments.Ids()) | ||
| } | ||
|
|
||
| // Get plugin-provided environment data for all envs |
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.
this comment talks about what is done above. can you move it to the top of the corresponding code snippet?
…ntegrated plugins
No description provided.