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

Classes to encapsulate Grok & ImageMagick system calls #146

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Conversation

mwarin
Copy link
Contributor

@mwarin mwarin commented Jul 31, 2024

This PR mainly moves some code out of inline system() calls and into more "proper" perl classes. There isn't much (any?) functional change... we're still doing the same things, just using different code to do it.

  • Tidied a bit
  • Broke out system calls from HTFeed::Stage::ImageRemediate into new classes HTFeed::Image::Grok and HTFeed::Image::Magick (plus some shared functionality in HTFeed::Image::Shared)
  • Added tests for new classes
  • Added new dev tool / test util: fail_fast.sh
  • Added a directory for reference images, thinking this will make it easier to test image operations

Review request

I'd like eyeballs on the 2 new classes Grok & Magick & new test image.t.
Check that you can build & test green:

git clone https://github.com/hathitrust/feed.git
cd feed
docker compose build
docker compose run --rm test bash t/fail_fast.sh t/*.t

The fail_fast.sh output should, if all was OK, end with:

volume.t : t/volume.t

@mwarin mwarin changed the title new class to encapsulate grok system calls Class to encapsulate Grok compress/decompress system calls Jul 31, 2024
@mwarin mwarin changed the title Class to encapsulate Grok compress/decompress system calls Classes to encapsulate Grok & ImageMagick system calls Jul 31, 2024
@mwarin mwarin force-pushed the DEV-1316 branch 2 times, most recently from 54d72e9 to 90fd431 Compare August 2, 2024 16:51
@mwarin mwarin requested review from aelkiss and liseli and removed request for aelkiss August 2, 2024 18:07
@mwarin mwarin marked this pull request as ready for review August 2, 2024 18:07
Copy link

@liseli liseli left a comment

Choose a reason for hiding this comment

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

Executing this command docker compose run --rm test bash t/fail_fast.sh t/*.t

I see the error below. Although it seems you did not update the t/classtests.t script, this test could be impacted with four changes.

lisepul@2379591497 feed % docker compose run --rm test bash t/fail_fast.sh t/*.t [+] Creating 4/0 ✔ Container feed-rabbitmq-1 Running 0.0s ✔ Container feed-pushgateway-1 Running 0.0s ✔ Container feed-minio-1 Running 0.0s ✔ Container feed-mariadb-1 Running 0.0s 001_load.t : t/001_load.t 002_load_all.t : t/002_load_all.t backup_expiration.t : t/backup_expiration.t gpg: error opening lockfile '/usr/local/feed/.gnupg/pubring.kbx.lock': No such file or directory gpg: lockfile disappeared bunnies.t : t/bunnies.t classtests.t : t/classtests.t HTFeed::Namespace::Test is not Test::Class or integer at t/classtests.t line 26. Error at t/classtests.t see /tmp/perltest_results/classtests.t

@mwarin
Copy link
Contributor Author

mwarin commented Aug 5, 2024

@liseli I don't understand why that error would happen, and you're right that's not part of the changed files.
When confronted by mysterious errors I usually blame the cache, nuke the cache, and run the tests again.
You could also run the tests in the same way as .github/workflows/tests.yml :

docker compose run test-and-cover

Copy link

@liseli liseli left a comment

Choose a reason for hiding this comment

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

Due to some unexpected issues, I could not run the tests locally. Martin and I set together to try to fix but it seems so complex. This PR does not introduce new features, it only gets a better organization of the functionalities. The change makes sense to me and I will approve it.

@mwarin mwarin merged commit 4216d9c into main Aug 19, 2024
1 check passed
@mwarin mwarin deleted the DEV-1316 branch August 19, 2024 14:57
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.

2 participants