Skip to content
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

#6362: only return unique services from getUnconfiguredComponentIntegrations #6364

Conversation

grahamlangford
Copy link
Collaborator

@grahamlangford grahamlangford commented Aug 28, 2023

@codecov
Copy link

codecov bot commented Aug 28, 2023

Codecov Report

Merging #6364 (291a38b) into main (8f60c22) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #6364      +/-   ##
==========================================
+ Coverage   69.94%   69.95%   +0.01%     
==========================================
  Files        1161     1161              
  Lines       36286    36295       +9     
  Branches     6826     6828       +2     
==========================================
+ Hits        25379    25391      +12     
+ Misses      10907    10904       -3     
Files Changed Coverage Δ
src/utils/modDefinitionUtils.ts 92.98% <100.00%> (+15.89%) ⬆️

... and 3 files with indirect coverage changes

}
return uniqBy(
modComponentDefinitions.flatMap(({ services }) => {
if (isEmpty(services)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional nit: could we rename services to integrations while we're at it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still referred/saved as services in the schema. That's a larger change than I'm comfortable making now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you're right. Wasn't thinking about the destructuring there 🤦‍♀️

if (isEmpty(services)) {
return [];
}
return uniqBy(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solution won't work as a non-required integration that is earlier in the array will be kept instead of a later required version. Rethinking the implmentation.

@@ -16,7 +16,31 @@
*/

import { type RegistryId } from "@/types/registryTypes";
import { getScopeAndId } from "@/utils/registryUtils";
import { generatePackageId, getScopeAndId } from "@/utils/registryUtils";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relocated test from modDefinitionUtils.test.ts

import { validateOutputKey } from "@/runtime/runtimeTypes";
import { validateRegistryId } from "@/types/helpers";
import { SERVICE_BASE_SCHEMA } from "@/services/serviceUtils";

extensionPointRegistry.lookup = jest.fn();

describe("generateRecipeId", () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

relocated to registryUtils.test.ts

for (const group of Object.values(groupBy(integrationDependencies, "id"))) {
if (group.some(({ isOptional }) => !isOptional)) {
dedupedIntegrationDependencies.push(
group.find(({ isOptional }) => !isOptional)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: any reason why not just assign the result of group.find to a constant above and use it in the if statement?

@github-actions
Copy link

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

@grahamlangford grahamlangford merged commit 5fb9a07 into main Aug 28, 2023
26 checks passed
@grahamlangford grahamlangford deleted the 6362-integration-configuration-selector-shows-multiple-times-for-integration-used-by-multiple-starter-bricks branch August 28, 2023 21:51
@grahamlangford grahamlangford added this to the 1.7.38 milestone Aug 28, 2023
grahamlangford added a commit that referenced this pull request Aug 28, 2023
…rations (#6364)

* only return unique services from getUnconfiguredComponentIntegrations

* updates logic for getUnconfiguredComponentIntegrations; adds tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integration configuration selector shows multiple times for integration used by multiple starter bricks
2 participants