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

Show error message if backing up credential file fails #2517

Merged
merged 8 commits into from
Jan 9, 2025

Conversation

dotNomad
Copy link
Collaborator

@dotNomad dotNomad commented Jan 8, 2025

This PR adds a new AgentError that bubbles up if operations in the backing-up of the credential file fail, showing the user the message so they may take action on it.

Intent

Resolves #2513

Type of Change

    • Bug Fix
    • New Feature
    • Breaking Change
    • Documentation
    • Refactor
    • Tooling

User Impact

Now a notification will be shown with the specific error that occurred when backing up a credential file, with an error code that we can do more with in the future.

Automated Tests

Automated tests were added to ensure that the correct data is sent back when the reset API errors.

Directions for Reviewers

Ensure that the error bubbles up via the API, is shown as a notification, and works in Workbench.

@sagerb
Copy link
Collaborator

sagerb commented Jan 9, 2025

I verified this functionality on the mac, by doing the following. It is easier to make these modifications to the codebase temporarily than debug the change on workbench. I'm confident that the functionality is the same.

  1. I modified the code to eliminating keyring support, and started VSCode with the extension
  2. I created a valid credential using the UX, so I had a valid .connect-credentials file and exited VSCode.
  3. I replaced it with a file with ZZZ in it (using echo "zza" > .connect-credentials)
  4. I then made it write-only with the command chmod -r .connect-credentials.

On launching and activating the extension, I then saw the new error message, with the low-level details of the OS call, which is what we wanted:
2025-01-09 at 11 07 AM

I do feel that we need to update the messaging on this error, as we're not communicating the context to the user that their credentials are corrupt (and we can't do anything about it).

@dotNomad
Copy link
Collaborator Author

dotNomad commented Jan 9, 2025

I do feel that we need to update the messaging on this error, as we're not communicating the context to the user that their credentials are corrupt (and we can't do anything about it).

Fantastic point. Let me get that messaging tweaked, and I'll request a re-review.

Copy link
Collaborator

@sagerb sagerb left a comment

Choose a reason for hiding this comment

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

Existing code looks great, but as mentioned in my verification note, we should update the error message to indicate that the user's credentials are corrupt and provide some action path for them to resolve.

Currently:

Failed to backup credentials to /Users/billsager/.connect-credentials-2025-01-09: open /Users/billsager/.connect-credentials: permission denied

Perhaps:

Corrupted credentials data found. Failed to resolve this issue by backing up credentials to /Users/billsager/.connect-credentials-2025-01-09: open /Users/billsager/.connect-credentials: permission denied" prior to resetting.

(Just a start ^^^)

@dotNomad
Copy link
Collaborator Author

dotNomad commented Jan 9, 2025

Existing code looks great, but as mentioned in my verification note, we should update the error message to indicate that the user's credentials are corrupt and provide some action path for them to resolve.

Pushed up a change. Now the error will read:

Unrecognizable credentials for Posit Publisher were found. Failed to backup credentials to /Users/billsager/.connect-credentials-2025-01-09: open /Users/billsager/.connect-credentials: permission denied

@dotNomad dotNomad requested a review from sagerb January 9, 2025 19:20
Copy link
Collaborator

@sagerb sagerb left a comment

Choose a reason for hiding this comment

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

LGTM

@dotNomad dotNomad merged commit 86c10ea into main Jan 9, 2025
14 checks passed
@dotNomad dotNomad deleted the dotnomad/creds-backup-err branch January 9, 2025 19:34
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.

Return meaningful error message when encountering low-level errors when resetting credentials
2 participants