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

Added exporter #90

Merged
merged 8 commits into from
Sep 23, 2022
Merged

Added exporter #90

merged 8 commits into from
Sep 23, 2022

Conversation

fadiga
Copy link
Collaborator

@fadiga fadiga commented Sep 3, 2022

Fixes #77

@fadiga fadiga added this to the M2 milestone Sep 3, 2022
@fadiga fadiga requested a review from rgaudin September 3, 2022 17:04
@fadiga fadiga self-assigned this Sep 3, 2022
@kelson42
Copy link
Contributor

kelson42 commented Sep 3, 2022

At which stage the quality assessment of this new feed will be checked (against current one)?

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (7354663) compared to base (237bd5e).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 7354663 differs from pull request most recent head b045584. Consider uploading reports for the commit b045584 to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #90   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        17    +2     
  Lines          544       567   +23     
=========================================
+ Hits           544       567   +23     
Impacted Files Coverage Δ
backend/src/backend/constants.py 100.00% <100.00%> (ø)
backend/src/backend/exporters/__init__.py 100.00% <100.00%> (ø)
...end/src/backend/exporters/kiwix_public_exporter.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

See inline comments.
Also, there's a conflict that you should look into (rebase from master) as nobody else works on this.

I'll add a ticket to add a trigger for this as we'll need to test this live as well

backend/src/backend/exporters/__init__.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/__init__.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/__init__.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/tests/test_exporter.py Outdated Show resolved Hide resolved
@fadiga fadiga requested a review from rgaudin September 7, 2022 12:38
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

I believe we agreed on testing the check_and_upload_file call in the tests like missing file to upload or incorrect UPLOAD_URI.

I also notice that there are warnings in the tests output regarding calls that were not awaited. This should be fixed (although it may originate from previous code).

Making sure upload status is reported is important ; see inline comments

backend/src/backend/constants.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/__init__.py Outdated Show resolved Hide resolved
backend/tests/test_exporter.py Outdated Show resolved Hide resolved
@fadiga fadiga requested a review from rgaudin September 13, 2022 15:41
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

See inline comments

backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/tests/conftest.py Show resolved Hide resolved
backend/tests/test_exporter.py Show resolved Hide resolved
@fadiga fadiga requested a review from rgaudin September 20, 2022 20:26
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

Thanks ; it's almost there but we need to test that common problems (namely our inputs) are caught, otherwise tests are not as useful:

  • incorrect URL should fail
  • incorrect RSA key should fail

backend/src/backend/exporters/kiwix_public_exporter.py Outdated Show resolved Hide resolved
backend/tests/test_exporter.py Outdated Show resolved Hide resolved
Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

See inline comment ; either the test or its name is incorrect

backend/tests/test_exporter.py Outdated Show resolved Hide resolved
@rgaudin rgaudin merged commit d64d4ac into main Sep 23, 2022
@rgaudin rgaudin deleted the issue77 branch September 23, 2022 16:59
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.

Exporter mechanism and Public Kiwix XML Export
3 participants