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

feat!: Remove legacy Sipi upload mechanism (DEV-4260) #3414

Merged
merged 25 commits into from
Nov 25, 2024

Conversation

siers
Copy link
Contributor

@siers siers commented Nov 4, 2024

Removed:

  • upload.lua
  • AssetIngestState
  • doSipiPostUpdate/doSipiPostUpdateIfTemp

Copy link

linear bot commented Nov 4, 2024

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.88%. Comparing base (6ebe8e7) to head (f10a5d1).
Report is 127 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3414       +/-   ##
===========================================
- Coverage   83.04%   17.88%   -65.16%     
===========================================
  Files         290      304       +14     
  Lines       23087    22760      -327     
===========================================
- Hits        19172     4071    -15101     
- Misses       3915    18689    +14774     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@siers siers force-pushed the fix/DEV-4260-remove-sipi-upload branch 5 times, most recently from 559609b to 95afa29 Compare November 14, 2024 14:47
@siers siers force-pushed the fix/DEV-4260-remove-sipi-upload branch 3 times, most recently from ce62660 to 43606ad Compare November 19, 2024 14:40
@siers siers requested a review from seakayone November 19, 2024 16:41
Copy link
Contributor

@seakayone seakayone left a comment

Choose a reason for hiding this comment

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

I would have expected the org.knora.webapi.routing.v2.AssetIngestState and related code to be removed as well.

This would also include removing the header and updating the documentation.

Are planning to do this in a followup PR?

@siers
Copy link
Contributor Author

siers commented Nov 20, 2024

I'll remove it here.

@siers siers force-pushed the fix/DEV-4260-remove-sipi-upload branch from 12552c0 to eb57f9c Compare November 20, 2024 15:11
Copy link
Contributor

@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.

Shouldn't be some removed tests rewritten to pointing to dsp-ingest?
Also please change the PR prefix to refactor.

@siers siers force-pushed the fix/DEV-4260-remove-sipi-upload branch 3 times, most recently from 5f28159 to f396b25 Compare November 22, 2024 10:32
@siers siers requested a review from seakayone November 22, 2024 10:55
Copy link
Contributor

@seakayone seakayone left a comment

Choose a reason for hiding this comment

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

Nice, overall it looks good to me. I have just some minor issues and suggestion

Just a couple of places where you can look into; I have searched for the ingested header and found these:

❯ rg AssetIngested
integration/src/test/scala/org/knora/webapi/it/v2/StandoffRouteV2ITSpec.scala
54:  private val addAssetIngested  = addHeader("X-Asset-Ingested", "true")
332:      ) ~> addCredentials(BasicHttpCredentials(anythingUserEmail, password)) ~> addAssetIngested

integration/src/test/scala/org/knora/webapi/it/v2/KnoraSipiIntegrationV2ITSpec.scala
67:  private val addAssetIngested = addHeader("X-Asset-Ingested", "true")
331:    getResponseJsonLD(request ~> addAuthorization ~> addAssetIngested)
❯ rg Asset-Ingested
CHANGELOG.md
341:* X-Asset-Ingested: skip Sipi temp move for updates (DEV-3504) ([#3219](https://github.com/dasch-swiss/dsp-api/issues/3219)) ([8cf4e9d](https://github.com/dasch-swiss/dsp-api/commit/8cf4e9dc5c93261179c1e6fa927448e84899a67f))

docs/03-endpoints/api-v2/editing-values.md
307:it is necessary to send the header `X-Asset-Ingested`

integration/src/test/scala/org/knora/webapi/e2e/v2/ValuesV2R2RSpec.scala
132:      val header = addHeader("X-Asset-Ingested", "true")

integration/src/test/scala/org/knora/webapi/it/v2/StandoffRouteV2ITSpec.scala
54:  private val addAssetIngested  = addHeader("X-Asset-Ingested", "true")
99:    ) ~> addHeader("X-Asset-Ingested", "true")

integration/src/test/scala/org/knora/webapi/it/v2/KnoraSipiIntegrationV2ITSpec.scala
67:  private val addAssetIngested = addHeader("X-Asset-Ingested", "true")

@seakayone
Copy link
Contributor

Shouldn't be some removed tests rewritten to pointing to dsp-ingest? Also please change the PR prefix to refactor.

Technically this is a breaking change and not a refactor. I would suggest feat!: Remove upload to Sipi and replace with dsp-ingest

@siers siers force-pushed the fix/DEV-4260-remove-sipi-upload branch from f85c89e to 3708cb6 Compare November 25, 2024 10:35
@siers siers requested a review from seakayone November 25, 2024 10:35
@siers siers merged commit b74a33c into main Nov 25, 2024
9 checks passed
@siers siers deleted the fix/DEV-4260-remove-sipi-upload branch November 25, 2024 12:23
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.

3 participants