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

Adding Netbird GitHub Link to the Client UI About Sub Menu #3372

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

robertgro
Copy link

@robertgro robertgro commented Feb 22, 2025

Describe your changes

Adding Netbird GitHub Link to the Client UI About Sub Menu

Issue ticket number and link

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

Mandatory in my opinion. You are free to decline/reject this PR if you like or if it isn't qualified (a reason would be nice then). Thanks anyway for the efforts guys.

@CLAassistant
Copy link

CLAassistant commented Feb 22, 2025

CLA assistant check
All committers have signed the CLA.

@robertgro
Copy link
Author

Quality Gate Failed Quality Gate failed

Failed conditions 1 New issue 9.52% Technical Debt Ratio on New Code (required ≤ 5%) 1 New Code Smells (required ≤ 0) B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

The issue here is that "This function has 104 lines of code, which is greater than the 100 authorized. Split it into smaller functions."

In my opinion, it would be a disproportionate expense to split and refactor this due to the limit. Either you have some space to ignore this limitation and approve the PR. On the other hand, it is understandable for me if this doesn't get merged into main (due to it shouldn't be that high of a priority to add such a link). Thx anyway as I said.

Lint and spacing fix for the GitHub Link sub menu entry
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 New issue
B Maintainability Rating on New Code (required ≥ A)
9.52% Technical Debt Ratio on New Code (required ≤ 5%)
1 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@robertgro
Copy link
Author

I gotta admit, it's rather a proposal than an actual implementation (was done on the fly). Feel free to alter the few lines of code as you like.

The lint issues have been corrected as well as the spacing in my editor.

For the line limitation of 100 lines in the func onTrayReady() https://github.com/netbirdio/netbird/blob/main/client/ui/client_ui.go#L576

you could consider to outsource the setup of the system tray menu items in https://github.com/netbirdio/netbird/blob/main/client/ui/client_ui.go#L580 due to by the time passing, you eventually would hit the limit anyways as the application begins to grow and more and more menu items are added.

This is a common software engineering pattern that is been found in frameworks like ASP.NET Core or Framework (compare Microsoft's dotnet approach in one of their project templates https://github.com/dotnet/templating/wiki/Available-templates-for-dotnet-new).

I'll leave this decision to your hands.

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