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

Desktop: Fixes #5875: Show error on sync if S3 region is not set #5923

Merged
merged 7 commits into from Jan 9, 2022

Conversation

shinglyu
Copy link
Contributor

Shows an error message when syncing if the S3 region is not set
image

Also shows a message if you try to check sync config in settings:
screenshot_2021-12-27_15-28-33

I also turn a few error log from info to error.
This should fix #5875

@shinglyu shinglyu mentioned this pull request Dec 27, 2021
@shinglyu
Copy link
Contributor Author

It was only tested on Linux desktop app. I'm not sure if this affects other platforms. I don't have other platforms for testing.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

My comments inline

packages/lib/commands/synchronize.ts Outdated Show resolved Hide resolved
packages/lib/components/shared/side-menu-shared.js Outdated Show resolved Hide resolved
shinglyu and others added 2 commits December 28, 2021 10:08
Co-authored-by: Helmut K. C. Tessarek <tessarek@evermeet.cx>
Co-authored-by: Helmut K. C. Tessarek <tessarek@evermeet.cx>
Copy link
Owner

@laurent22 laurent22 left a comment

Choose a reason for hiding this comment

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

That's great, thanks for the fix @shinglyu. I've just added a comment below.

reg.logger().error(error);
utils.store.dispatch({
type: 'SYNC_REPORT_UPDATE',
report: { errors: [error] },
Copy link
Owner

Choose a reason for hiding this comment

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

As it's going to affect all sync targets, and some might output not so useful error messages, please could you add more info? Just error.message = "Could not initialise synchroniser: " + error.message should do.

And that should be done in side-menu-shared too.

Copy link
Owner

Choose a reason for hiding this comment

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

"Could not initialise synchroniser" please. We use British English by default and "acquire synchronizer" doesn't mean much I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@laurent22 Thank you for the comment. All the logs are now in British English.

@tessus tessus added the desktop All desktop platforms label Dec 28, 2021
@tessus tessus added the sync sync related issue label Jan 6, 2022
@laurent22
Copy link
Owner

Looks good now, thanks for the fix @shinglyu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms sync sync related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S3 sync issues in 2.6
3 participants