Skip to content

Conversation

@justonedev1
Copy link
Collaborator

No description provided.

@justonedev1 justonedev1 requested a review from knopers8 as a code owner June 3, 2025 07:51
message GetEnvironmentsRequest {
bool showAll = 1;
bool showTaskInfos = 2;
bool showIntegratedServices = 3;
Copy link
Collaborator

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
Comment on lines 297 to 301

if request.GetShowIntegratedServices() {
e.IntegratedServicesData = integration.PluginsInstance().GetEnvironmentsData([]uid.ID{env.Id()})[env.Id()]
}

Copy link
Collaborator

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
Copy link
Collaborator

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?

@knopers8 knopers8 merged commit 6a9a18c into master Jun 4, 2025
4 checks passed
@knopers8 knopers8 deleted the OCTRL-1012 branch June 4, 2025 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants