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

Add title when adding public keys #1171

Merged
merged 6 commits into from
Dec 18, 2024
Merged

Add title when adding public keys #1171

merged 6 commits into from
Dec 18, 2024

Conversation

moalshak
Copy link
Contributor

@moalshak moalshak commented Dec 14, 2024

This PR adds the title input as a field in the frontend and saves it in the DB.

A view from the modal:

Screenshot 2024-12-15 at 12 43 59 AM

Public key after being saved:

Screenshot 2024-12-15 at 12 46 21 AM

Working on this, I realized that we should probably add more metadata like when it was added, when it was last used etc.

Fixes: #714
Fixes: #754
Fixes: #898
Fixes: #1176

One thing to note is that my passwords manager stopped recognizing the public id field as one.

Copy link
Member

@Eugeny Eugeny left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

Working on this, I realized that we should probably add more metadata like when it was added, when it was last used etc.

That's a great idea - happy to merge if you're willing to work on this.

passwords manager stopped recognizing the public id field as one.

What does this mean?

@moalshak
Copy link
Contributor Author

What does this mean?

My 1Password used to recognize the field as an ssh field and suggest adding my saved pub key in it.

image

@Eugeny
Copy link
Member

Eugeny commented Dec 15, 2024

Oh - I wonder what logic it uses to recognize it.

@moalshak
Copy link
Contributor Author

Oh - I wonder what logic it uses to recognize it.

Not great logic tbh haha, since it thinks the title input is the ssh input on the https://github.com/settings/ssh/new page.

@moalshak
Copy link
Contributor Author

moalshak commented Dec 17, 2024

@Eugeny The tests step seems to fail with the following trace:

20:32:05.854 ERROR Error during SonarScanner CLI execution
java.lang.IllegalStateException: Error status returned by url [https://api.sonarcloud.io/analysis/jres?os=linux&arch=x86_64]: 401
	at org.sonarsource.scanner.lib.internal.http.ScannerHttpClient.callUrl(ScannerHttpClient.java:163)
	at org.sonarsource.scanner.lib.internal.http.ScannerHttpClient.callApi(ScannerHttpClient.java:126)
	at org.sonarsource.scanner.lib.internal.http.ScannerHttpClient.callRestApi(ScannerHttpClient.java:104)
	at org.sonarsource.scanner.lib.internal.JavaRunnerFactory.getJreMetadata(JavaRunnerFactory.java:159)
	at org.sonarsource.scanner.lib.internal.JavaRunnerFactory.getJreFromServer(JavaRunnerFactory.java:138)
	at org.sonarsource.scanner.lib.internal.JavaRunnerFactory.createRunner(JavaRunnerFactory.java:85)
	at org.sonarsource.scanner.lib.internal.ScannerEngineLauncherFactory.createLauncher(ScannerEngineLauncherFactory.java:53)
	at org.sonarsource.scanner.lib.ScannerEngineBootstrapper.bootstrap(ScannerEngineBootstrapper.java:123)
	at org.sonarsource.scanner.cli.Main.analyze(Main.java:75)
	at org.sonarsource.scanner.cli.Main.main(Main.java:63)
20:32:05.855 ERROR

Is this something I can fix?

@Eugeny
Copy link
Member

Eugeny commented Dec 17, 2024

You can ignore the sonarqube error - however the migration is failing when there are existing keys in the database:

23:11:50 ERROR Fatal error error=Execution Error: error returned from database: (code: 1) Cannot add a NOT NULL column with default value NULL

@moalshak
Copy link
Contributor Author

moalshak commented Dec 18, 2024

@Eugeny I fixed that (locally). However, I am running into issues now due to the name "label". In the user's view of the profile, that field was already being used for the abbreviated ssh key. See:

Screenshot 2024-12-18 at 9 45 35 AM Screenshot 2024-12-18 at 9 45 49 AM

What do you propose to change ? The name from label to back to title ? Or the old label to abbreviation?

@Eugeny
Copy link
Member

Eugeny commented Dec 18, 2024

Let's change the old label to abbreviated

also fix db migration
also fix warp-tech#1176
@Eugeny
Copy link
Member

Eugeny commented Dec 18, 2024

Looks good now - thanks!

@all-contributors add @moalshak for code

@Eugeny Eugeny merged commit 1dec4c9 into warp-tech:main Dec 18, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants