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.

Sign up for GitHub

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

Jump to bottom

SLVS-1407 Integrate BoundServerProject with IBindingProcess #5655

Merged
merged 7 commits into from
Sep 3, 2024
Merged

SLVS-1407 Integrate BoundServerProject with IBindingProcess #5655

merged 7 commits into from
Sep 3, 2024

Conversation

georgii-borovinskikh-sonarsource
Copy link
Contributor

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource commented Aug 30, 2024 •
edited by jira bot
Loading

SLVS-1407

Part of

georgii-borovinskikh-sonarsource
georgii-borovinskikh-sonarsource commented Aug 30, 2024
View reviewed changes
src/ConnectedMode/Persistence/ConnectionInfoConverter.cs

namespace SonarLint.VisualStudio.ConnectedMode.Persistence;

[ExcludeFromCodeCoverage] // todo remove https://sonarsource.atlassian.net/browse/SLVS-1408
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is added temporarily

georgii-borovinskikh-sonarsource
georgii-borovinskikh-sonarsource commented Aug 30, 2024
View reviewed changes
src/ConnectedMode/Binding/IUnintrusiveBindingController.cs
@@ -29,32 +30,57 @@ namespace SonarLint.VisualStudio.ConnectedMode.Binding
internal interface IUnintrusiveBindingController
{
Task BindAsync(BoundSonarQubeProject project, IProgress<FixedStepsProgress> progress, CancellationToken token);
Task BindAsync(BoundServerProject project, IProgress<FixedStepsProgress> progress, CancellationToken token);
}

[Export(typeof(IUnintrusiveBindingController))]
[PartCreationPolicy(CreationPolicy.NonShared)]
internal class UnintrusiveBindingController : IUnintrusiveBindingController
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class & interface is the main thing about this PR

georgii-borovinskikh-sonarsource
georgii-borovinskikh-sonarsource commented Aug 30, 2024
View reviewed changes
src/ConnectedMode/Binding/BindCommandArgs.cs
public BindCommandArgs(string projectKey, string projectName, ConnectionInformation connection)
public BoundServerProject ProjectToBind { get; }

public BindCommandArgs(BoundServerProject projectToBind)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the args as the new UI will operate with proper models

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource marked this pull request as ready for review August 30, 2024 13:39
@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource deleted the branch feature/new-connected-mode August 30, 2024 13:52
@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource deleted the branch feature/new-connected-mode August 30, 2024 13:58
@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource force-pushed the gb/binding-process-server-connection branch from ed9e1b9 to c5a6bab Compare August 30, 2024 14:02
gabriela-trutan-sonarsource
gabriela-trutan-sonarsource approved these changes Sep 2, 2024
View reviewed changes
src/Core/Binding/BoundServerProject.cs Outdated

return new BoundServerProject("Solution Name Placeholder", // todo https://sonarsource.atlassian.net/browse/SLVS-1422
public static BoundServerProject FromBoundSonarQubeProject(BoundSonarQubeProject boundProject, string localBindingKey = null, ServerConnection connection = null) =>
new(localBindingKey ?? "Solution Name Placeholder", // todo https://sonarsource.atlassian.net/browse/SLVS-1422
Copy link
Contributor

Choose a reason for hiding this comment

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

This task is already resolved. Was another task intended here? Maybe https://sonarsource.atlassian.net/browse/SLVS-1408

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't have been resolved. I think I mentioned it in one of the other PRs and the system thought it was resolved. I unresolved it

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, there was a PR that was approved. Not sure why are we ressurecting the ticket?

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 mixed it up with the other ticket, re-closed it. I will replace the comment, as it should point to the repository migration ticket.

src/ConnectedMode/Binding/BindCommandArgs.cs
@@ -27,17 +28,11 @@ namespace SonarLint.VisualStudio.ConnectedMode.Binding
/// </summary>
public class BindCommandArgs
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can completely drop the BindCommandArgs class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will. Now it's there to cause less compile errors. I will add this to the description of the task for the cleanup

src/ConnectedMode/Binding/IUnintrusiveBindingController.cs Outdated

if (!serverConnectionsRepository.TryGet(proposedConnection.Id, out var connection))
{
if (!serverConnectionsRepository.TryAdd(proposedConnection))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should document here why we are adding the connection, in case it doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We add it because it doesn't exist and it's a migration process🙃

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly that you should document.

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 can rename the method to mention migration specifically.

src/ConnectedMode/Binding/IUnintrusiveBindingController.cs
@@ -29,32 +30,57 @@ namespace SonarLint.VisualStudio.ConnectedMode.Binding
internal interface IUnintrusiveBindingController
{
Task BindAsync(BoundSonarQubeProject project, IProgress<FixedStepsProgress> progress, CancellationToken token);
Task BindAsync(BoundServerProject project, IProgress<FixedStepsProgress> progress, CancellationToken token);
}

[Export(typeof(IUnintrusiveBindingController))]
[PartCreationPolicy(CreationPolicy.NonShared)]
internal class UnintrusiveBindingController : IUnintrusiveBindingController
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this class called "Unintrusive"? Should we change the name? If not, maybe we should add a documentation to the interface to explain the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unintrusive = does not add anything into the csproj files or other git tracked files

Copy link
Contributor

Choose a reason for hiding this comment

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

Please document that

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 think it makes sense to leave a comment to explain a pre-existing naming convention. This is not the only class that uses Unintrusive in it's name and It was not renamed in this PR. The word was introduced in version 7.0 to distinguish it from the Obsolete binding (IObsoleteBindingConfigurationProvider).

@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Sep 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 85%)

See analysis details on SonarCloud

@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource merged commit 09c0e6a into feature/new-connected-mode Sep 3, 2024
1 of 2 checks passed
@georgii-borovinskikh-sonarsource georgii-borovinskikh-sonarsource deleted the gb/binding-process-server-connection branch September 3, 2024 08:52
gabriela-trutan-sonarsource pushed a commit that referenced this pull request Sep 3, 2024
@georgii-borovinskikh-sonarsource @gabriela-trutan-sonarsource
SLVS-1407 Integrate BoundServerProject with IBindingProcess (#5655)
ff426a6 
[SLVS-1407](https://sonarsource.atlassian.net/browse/SLVS-1407)

Part of <!-- 
  Only for standalone PRs without Jira issue in the PR title: 
    * Replace this comment with Epic ID to create a new Task in Jira
* Replace this comment with Issue ID to create a new Sub-Task in Jira
* Ignore or delete this note to create a new Task in Jira without a
parent
-->


[SLVS-1407]:
https://sonarsource.atlassian.net/browse/SLVS-1407?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
vnaskos-sonar pushed a commit that referenced this pull request Sep 12, 2024
@georgii-borovinskikh-sonarsource @vnaskos-sonar
SLVS-1407 Integrate BoundServerProject with IBindingProcess (#5655)
331fa32 
[SLVS-1407](https://sonarsource.atlassian.net/browse/SLVS-1407)

Part of <!-- 
  Only for standalone PRs without Jira issue in the PR title: 
    * Replace this comment with Epic ID to create a new Task in Jira
* Replace this comment with Issue ID to create a new Sub-Task in Jira
* Ignore or delete this note to create a new Task in Jira without a
parent
-->


[SLVS-1407]:
https://sonarsource.atlassian.net/browse/SLVS-1407?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
vnaskos-sonar pushed a commit that referenced this pull request Sep 19, 2024
@georgii-borovinskikh-sonarsource @vnaskos-sonar
SLVS-1407 Integrate BoundServerProject with IBindingProcess (#5655)
4af1c03 
[SLVS-1407](https://sonarsource.atlassian.net/browse/SLVS-1407)

Part of <!-- 
  Only for standalone PRs without Jira issue in the PR title: 
    * Replace this comment with Epic ID to create a new Task in Jira
* Replace this comment with Issue ID to create a new Sub-Task in Jira
* Ignore or delete this note to create a new Task in Jira without a
parent
-->


[SLVS-1407]:
https://sonarsource.atlassian.net/browse/SLVS-1407?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
gabriela-trutan-sonarsource pushed a commit that referenced this pull request Sep 27, 2024
@georgii-borovinskikh-sonarsource @gabriela-trutan-sonarsource
SLVS-1407 Integrate BoundServerProject with IBindingProcess (#5655)
6c299cc 
[SLVS-1407](https://sonarsource.atlassian.net/browse/SLVS-1407)

Part of <!-- 
  Only for standalone PRs without Jira issue in the PR title: 
    * Replace this comment with Epic ID to create a new Task in Jira
* Replace this comment with Issue ID to create a new Sub-Task in Jira
* Ignore or delete this note to create a new Task in Jira without a
parent
-->


[SLVS-1407]:
https://sonarsource.atlassian.net/browse/SLVS-1407?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gabriela-trutan-sonarsource gabriela-trutan-sonarsource gabriela-trutan-sonarsource approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@georgii-borovinskikh-sonarsource @gabriela-trutan-sonarsource