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

SLVS-1818 Upgrade SLCore to 10.14.0.80189 and adapt RPC changes for SQC multi-region #5985

Merged
merged 5 commits into from
Feb 5, 2025

Conversation

georgii-borovinskikh-sonarsource
Copy link
Contributor

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource commented Feb 4, 2025

Copy link
Contributor

Choose a reason for hiding this comment

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

You missed adding the region for ListUserOrganizationsParams. Now we have an exception when trying to create a SQC connection.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some feedback regarding CaYC

@@ -176,7 +177,7 @@ public async Task GetOrganizationsAsync_TokenIsProvided_CallsSlCoreListUserOrgan

await testSubject.GetOrganizationsAsync(new TokenCredentialsModel(token.CreateSecureString()));

await connectionConfigurationSlCoreService.Received(1).ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x=> IsExpectedCredentials(x.credentials, token)));
await connectionConfigurationSlCoreService.Received(1).ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x=> IsExpectedCredentials(x.credentials, token) && x.region == SonarCloudRegion.EU));

Choose a reason for hiding this comment

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

Please add a helper method to check the region, i.e. IsExpectedRegion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really see the point of creating a method to replace one equals expression

Choose a reason for hiding this comment

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

Because this occurs in multiple places. And if something changes, it would be easier to just modify one helper method instead of multiple places.

@@ -192,10 +193,10 @@ public async Task GetOrganizationsAsync_CredentialsIsNull_ReturnsFailedResponseA
[TestMethod]
public async Task GetOrganizationsAsync_NoOrganizationExists_ReturnsSuccessResponseAndEmptyOrganizations()
{
connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Any<ListUserOrganizationsParams>())
connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x => x.credentials.Left.token == token && x.region == SonarCloudRegion.EU))

Choose a reason for hiding this comment

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

Suggested change
connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x => x.credentials.Left.token == token && x.region == SonarCloudRegion.EU))
connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x => IsExpectedCredentials(x.credentials, token) && IsExpectedRegion(x.region)))

@@ -205,10 +206,11 @@ public async Task GetOrganizationsAsync_NoOrganizationExists_ReturnsSuccessRespo
public async Task GetOrganizationsAsync_OrganizationExists_ReturnsSuccessResponseAndMappedOrganizations()
{
List<OrganizationDto> serverOrganizations = [new OrganizationDto("key", "name", "desc"), new OrganizationDto("key2", "name2", "desc2")];
connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Any<ListUserOrganizationsParams>())

connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x => x.credentials.Left.token == token && x.region == SonarCloudRegion.EU))

Choose a reason for hiding this comment

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

Suggested change
connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x => x.credentials.Left.token == token && x.region == SonarCloudRegion.EU))
connectionConfigurationSlCoreService.ListUserOrganizationsAsync(Arg.Is<ListUserOrganizationsParams>(x => IsExpectedCredentials(x.credentials, token) && IsExpectedRegion(x.region)))

@gabriela-trutan-sonarsource
Copy link
Contributor

@georgii-borovinskikh-sonarsource, this PR should target master.

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource changed the base branch from feature/sqc-multi-region to master February 5, 2025 11:23
@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource merged commit 3edccfa into master Feb 5, 2025
3 checks passed
@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource deleted the gb/slcore-10-14 branch February 5, 2025 11:47
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.

2 participants