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

Jump to bottom

refactor(IIIFService): zio-fying iiif service (DEV-801) #2044

Merged
merged 50 commits into from May 5, 2022
Merged

refactor(IIIFService): zio-fying iiif service (DEV-801) #2044

merged 50 commits into from May 5, 2022

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Apr 13, 2022 •
edited

Resolves DEV-801

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@subotic subotic self-assigned this Apr 13, 2022
@subotic
Merge branch 'main' into wip/DEV-801-zio-fying-iiif-service
5cfa753
@subotic
Merge branch 'main' into wip/DEV-801-zio-fying-iiif-service
1f1d6bc
@subotic
refactor(IIIFService): use TestManagersWithMockedSipi for tests needi…
755d62a 
…ng mocked Sipi
@subotic
build(CI): change Java to Temurin JDK 17
ea9a4dd
@subotic
refactor(IIIFService): use TestManagersWithAllTestContainers (ongoing)
2a18748
@subotic
refactor(IIIFService): use TestManagersWithAllTestContainers
bed2eff
@subotic
refactor(IIIFService): use sipi test client (ongoing)
76b92cb
@subotic
refactor(IIIFService): use test client (ongoing)
5d4c3ce
@subotic
refactor(IIIFService): use test client
09a07b9
@subotic
refactor(IIIFService): fix tests
8a90c9c
@subotic
refactor(IIIFService): fix tests
e9cf31e
@subotic
refactor(IIIFService): fix shacl tests
867e4f3
@subotic
refactor(IIIFService): fix integration specs (reverting to original i…
bdc03b9 
…mplementation)
@subotic
refactor(IIIFService): fix tests on CI (trying)
d2fa994
@subotic
Update TestClientService.scala
60bf122
@subotic
remove unused tests
e292672
@subotic
refactor(IIIFService): optimize logging
c54c77d
@subotic
refactor(IIIFService): raise timeout in TestClient
180c95e
@subotic
refactor(IIIFService): fix error handling
f842570
@subotic
refactor(IIIFService): tune down logging
86b658a
@subotic
add scalafmt plugin
7aea3de
@subotic
add better error handling at the edge
63461fa
@subotic
add automatic format checking
76b3434
@subotic
add automatic format checking
f390494
@subotic
add automatic format checking
6053b17
@subotic
Merge branch 'main' into wip/DEV-801-zio-fying-iiif-service
499c3f2
@subotic
format more files
6a5d77e
irinaschubert
irinaschubert approved these changes May 3, 2022
View reviewed changes
Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

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

LGTM although I can't really contribute anything regarding ZIO, just added some minor stuff (typos, questions).

webapi/src/main/resources/application.conf Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala Outdated Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala Outdated Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/exceptions/Exceptions.scala Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/store/StoreManager.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testcontainers/SipiTestContainer.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testservices/TestClientService.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testservices/TestClientService.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testservices/TestClientService.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testservices/TestClientService.scala Outdated Show resolved Hide resolved
BalduinLandolt
BalduinLandolt requested changes May 3, 2022
View reviewed changes
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

Mostly looks good and seems a big improvement, thanks!

I added some smaller remarks that you may want to consider.

Finally, I'm not sure why ValuesResponderV2Spec.scala does what it should. (See my comment there.) Please have a look at that.

project/plugins.sbt Outdated Show resolved Hide resolved
webapi/src/main/resources/application.conf Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala Outdated Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/store/iiif/impl/IIIFServiceSipiImpl.scala Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/E2ESpec.scala Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/store/iiif/impl/IIIFServiceMockSipiImpl.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/store/iiif/impl/IIIFServiceMockSipiImpl.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/store/iiif/impl/IIIFServiceMockSipiImpl.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testservices/TestClientService.scala Outdated Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testservices/TestClientService.scala Show resolved Hide resolved
subotic and others added 11 commits May 3, 2022 13:20
@subotic
Update webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala
f0e740f 
Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
@subotic
Update webapi/src/test/scala/org/knora/webapi/testcontainers/SipiTest…
f8d2f88 
…Container.scala

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
@subotic
Update webapi/src/test/scala/org/knora/webapi/testservices/TestClient…
937df12 
…Service.scala

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
@subotic
Update webapi/src/test/scala/org/knora/webapi/testservices/TestClient…
245b02a 
…Service.scala

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
@subotic
Update webapi/src/test/scala/org/knora/webapi/testservices/TestClient…
5de2b21 
…Service.scala

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
@subotic
Update webapi/src/test/scala/org/knora/webapi/testservices/TestClient…
5d064cc 
…Service.scala

Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
@subotic
add review fixes
ca5bd78
@subotic
Merge branch 'wip/DEV-801-zio-fying-iiif-service' of github.com:dasch…
9a2a8d4 
…-swiss/knora-api into wip/DEV-801-zio-fying-iiif-service
@subotic
add review fixes
024eec0
@subotic
fix formatting
8243aa1
@subotic
Update .gitignore
ce5929b
mpro7
mpro7 reviewed May 3, 2022
View reviewed changes
Copy link
Collaborator

@mpro7 mpro7 left a comment

Choose a reason for hiding this comment

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

Not everything is clear for me in this PR but found some issues related to code readability.

webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala Outdated Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/store/iiif/IIIFServiceManager.scala Outdated Show resolved Hide resolved
webapi/src/main/scala/org/knora/webapi/store/iiif/domain/SipiKnoraJsonResponse.scala Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/e2e/v2/ValuesV2R2RSpec.scala Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/it/v2/KnoraSipiIntegrationV2ITSpec.scala Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/responders/v1/ResourcesResponderV1Spec.scala Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala Show resolved Hide resolved
webapi/src/test/scala/org/knora/webapi/testservices/TestClientService.scala Outdated Show resolved Hide resolved
@BalduinLandolt BalduinLandolt self-requested a review May 5, 2022 06:55
@sonarcloud
Copy link

sonarcloud bot commented May 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
9.9% 9.9% Duplication

@subotic subotic merged commit 224b664 into main May 5, 2022
@subotic subotic deleted the wip/DEV-801-zio-fying-iiif-service branch May 5, 2022 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpro7 mpro7 mpro7 left review comments

@irinaschubert irinaschubert irinaschubert approved these changes

@BalduinLandolt BalduinLandolt BalduinLandolt approved these changes

Assignees

@subotic subotic

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@subotic @irinaschubert @mpro7 @BalduinLandolt