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(ingest-upload): create new ingest xmlupload cli command (DEV-3019) #670

Merged
merged 91 commits into from Dec 22, 2023

Conversation

Nora-Olivia-Ammann
Copy link
Collaborator

No description provided.

Copy link

linear bot commented Dec 5, 2023

@Nora-Olivia-Ammann Nora-Olivia-Ammann self-assigned this Dec 5, 2023
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.

looks goos! Just some minor remarks

src/dsp_tools/commands/ingest_xmlupload/upload_xml.py Outdated Show resolved Hide resolved
test/e2e/commands/test_ingest_xmlupload.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jnussbaum jnussbaum left a comment

Choose a reason for hiding this comment

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

Overall, it looks very good. Many of my comments are cosmetical, but some should be addressed before merging. They are commented, but here an overview:

  • type annotation of IngestInformation.media_no_id
  • should we disallow an xmlupload if there are files that were ingested but not in the XML?
  • wrong file given in _get_unused_path_msg()
  • We shouldn't add the header "X-Asset-Ingested" to every single POST request that DSP-TOOLS makes, but only if a resource is created via ingest-xmlupload.
  • unused test data files (should be removed)

src/dsp_tools/commands/ingest_xmlupload/apply_ingest_id.py Outdated Show resolved Hide resolved
src/dsp_tools/commands/ingest_xmlupload/apply_ingest_id.py Outdated Show resolved Hide resolved
src/dsp_tools/commands/ingest_xmlupload/apply_ingest_id.py Outdated Show resolved Hide resolved
src/dsp_tools/commands/ingest_xmlupload/apply_ingest_id.py Outdated Show resolved Hide resolved
testdata/dsp-ingest-data/images/GoodGirl.jpg Outdated Show resolved Hide resolved
testdata/dsp-ingest-data/images/subFolder/Fluffy.jpg Outdated Show resolved Hide resolved
testdata/dsp-ingest-data/images/subFolder/ShibaInu.jpg Outdated Show resolved Hide resolved
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.

I didn't do a detailed review now, but I feel we should try to get this merged soon-ish and try it out in practice. And if things need improvement after all, we can still do that in a next PR.

src/dsp_tools/utils/connection_live.py Show resolved Hide resolved
@Nora-Olivia-Ammann Nora-Olivia-Ammann merged commit 5745190 into main Dec 22, 2023
9 checks passed
@Nora-Olivia-Ammann Nora-Olivia-Ammann deleted the wip/dev-3019-create-new-ingest-cli branch December 22, 2023 15:33
@daschbot daschbot mentioned this pull request Dec 20, 2023
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

3 participants