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

handle s3 timeout on settings page #4036

Merged
merged 6 commits into from May 13, 2024
Merged

handle s3 timeout on settings page #4036

merged 6 commits into from May 13, 2024

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Apr 23, 2024

Changes

This PR reduces export-fetch request attempts to one and handles the resulting error (if there is one). This allows import feature to continue working when S3 (used in exports) is not available.

Screenshot 2024-04-23 at 23 06 22

Tests

  • Automated tests have been added

Changelog

  • This PR does not make a user-facing change

Documentation

  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode

@ruslandoga ruslandoga requested a review from zoldar April 23, 2024 15:09
assign(socket, status: "in_progress")
%{storage: storage, site: site, site_id: site_id} = socket.assigns

assign_async(socket, [:status, :export], fn ->
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 went with a quick-fix using assign_async. Alternative approach is reducing the number of attempts in Exports.get_s3_export/1 to 1 and adding one more :status for export_fetch_failure. That would remove the loading state which is somewhat unnecessary in "happy path".

Any preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine with assign_async 👍 Perhaps we could extract get_export assingment and Exports.get_last_export_job/1 outside of it? Not a big deal though.

@ruslandoga ruslandoga changed the title handle s3 timeout handle s3 timeout on settings page Apr 23, 2024
assign(socket, status: "in_progress")
%{storage: storage, site: site, site_id: site_id} = socket.assigns

assign_async(socket, [:status, :export], fn ->
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine with assign_async 👍 Perhaps we could extract get_export assingment and Exports.get_last_export_job/1 outside of it? Not a big deal though.

@ruslandoga ruslandoga marked this pull request as draft April 30, 2024 09:13
@ruslandoga ruslandoga marked this pull request as ready for review May 13, 2024 05:10
@ruslandoga ruslandoga requested a review from zoldar May 13, 2024 05:10
prev_env = Application.get_env(:ex_aws, :s3)
new_env = Keyword.update!(prev_env, :port, fn prev_port -> prev_port + 1 end)
Application.put_env(:ex_aws, :s3, new_env)
on_exit(fn -> Application.put_env(:ex_aws, :s3, prev_env) end)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

patch_env only supports :plausible app

end

@tag :capture_log
test "displays error message", %{conn: conn, site: site} do
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'll need to skip this test in CE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zoldar zoldar merged commit 7af8273 into master May 13, 2024
9 checks passed
@zoldar zoldar deleted the handle-ex-aws-s3-timeout branch May 13, 2024 08:17
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.

None yet

2 participants