DTS manifest file part 1 #205

briehl · 2024-11-13T22:47:40Z

This includes the first steps toward having the DTS manifest files as import specifications. It does the following:

Adds .json to the allowed import spec files, as DTS_MANIFEST
Maps DTS_MANIFEST to a (for now) dummy parser function.
Starts up the dummy parser function with basic fail conditions - file not found, file is a directory, file is wrong (basic) format, etc.

Next PR will make the parser do some parsing.

MrCreosote

It's been so long I don't really remember how the mapping mechanism works

MrCreosote · 2024-11-14T19:47:14Z

staging_service/autodetect/Mappings.py

+    DTS_MANIFEST: ["json"],
    JSON: ["json"],


DTS_MANIFEST and JSON are both the text JSON and so collide in the map

MrCreosote · 2024-11-14T19:49:46Z

Ugh, hit finish review too early. I'm not done

MrCreosote

Holding the review here as I think the app configuration is incorrect

MrCreosote · 2024-11-14T20:05:13Z

staging_service/autodetect/Mappings.py

+# Data Transfer Service Manifest format (which is a specific JSON format)
+DTS_MANIFEST = "JSON"
+


These types are just the file types, regardless of the semantics of the type. Just JSON should be used here

MrCreosote · 2024-11-14T20:06:22Z

deployment/conf/supported_apps_w_extensions.json

+        {
+          "id": "import_specification",
+          "title": "Import Specification",
+          "app_weight": 1


This file is supposed to be autogenerated IIRC but I don't see any code changes that would cause this to be different

IIRC you need to update the mapping here:

staging_service/staging_service/autodetect/GenerateMappings.py

Line 66 in d41d38c

file_format_to_app_mapping[JSON] = [escher_map_id]

Well that'd be real cool if that were documented anywhere. I'll add that and fix it.

Added some documentation to the README. Not perfect, but should be enough for now.

briehl · 2024-11-14T21:37:12Z

Ok, that was a very silly change I did.
New version:

Removed that DTS_MANIFEST mapping nonsense, just stick with JSON.
Ran GenerateMappings.py to update the mapping JSON file. Which really didn't change anything, but we should consider best practice.
Added some lightweight documentation on the file mapping stuff and how to update it. Should come back to that at some point.

briehl · 2024-11-14T21:38:39Z

Trivy's being whiny again: TOOMANYREQUESTS: retry-after: 483.512µs, allowed: 44000/minute"

MrCreosote · 2024-11-14T22:22:37Z

staging_service/autodetect/GenerateMappings.py

@@ -120,7 +129,7 @@
    fba_model_id,
    import_specification,
 ]
-file_format_to_app_mapping[JSON] = [escher_map_id]
+file_format_to_app_mapping[JSON] = [escher_map_id, import_specification]


Now I'm wondering if we should have a separate type in the dropdown for DTS manifests vs. import specifications. Maybe just DTS manifest? That would be forward compatible if we ever wanted to support standard import specifications with JSON input.

Yeah, I thought of that. I guess that would be a trivial change, but I'm not sure the best thing to call it. I also wonder if it would be confusing to users at all? They'd have to read instructions on the whole DTS process anyway.

Oh, I take it back, that's not trivial at all. It means a narrative change at minimum, possibly an API change if we want to be really thorough. Maybe a new API endpoint?

I would make it a query param probably

Just checking here - you're saying the separate type and query param is the way you're going to go?

Also, if it's going to work that way, there probably shouldn't be a JSON: dts_parser entry in the parser mapping

This is how it work right now, as of this PR, and everything else that's in it. A later PR that updates the endpoint to use a query param will probably undo this. Also, the Narrative will need to have a mapping of some sort, which is what this sets up. So it might not be from .json -> Import Specification, but it'll need to be something similar.

I don't want this to get more complicated and would like to keep that in a single PR with the bulk_specification endpoint change.

Fair enough, as long as the plan is clear

staging_service/autodetect/Mappings.py

staging_service/import_specifications/individual_parsers.py

sonarcloud · 2024-11-19T21:15:42Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

MrCreosote

LGTM given the caveats re reworking this have have a specific DTS manifest type

MrCreosote · 2024-11-19T21:24:04Z

tests/import_specifications/test_data/manifest_small.json

@@ -0,0 +1,122 @@
+{


This doesn't seem to match the documentation in individual_parsers but I assume the format is still changing

briehl added 7 commits November 11, 2024 13:50

add .json files (dts import manifest) to import specification list

a456122

add dummy parse_dts_manifest function

d2765a4

progress on dts_manifest base error tests

2f5b2d0

add dummy dts manifest parser and basic fail tests

ab6bbc7

add JSON format check and test

ae4a74d

fix test fixture docstrong

5d945ff

ruff formatting

ccf1761

briehl requested a review from MrCreosote November 13, 2024 22:47

MrCreosote reviewed Nov 14, 2024

View reviewed changes

MrCreosote requested changes Nov 14, 2024

View reviewed changes

briehl added 2 commits November 14, 2024 16:32

fix silliness around dts manifest mapping

4f6044d

add link from API call to instructions on how to update mappings

1e4fc01

briehl requested a review from MrCreosote November 14, 2024 21:37

MrCreosote reviewed Nov 14, 2024

View reviewed changes

briehl added 2 commits November 14, 2024 18:16

fix typo

1fc9a2c

update docstrings, add TODOS

a19d227

briehl requested a review from MrCreosote November 18, 2024 20:27

remove 'roughly'

bd55791

MrCreosote approved these changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DTS manifest file part 1 #205

DTS manifest file part 1 #205

briehl commented Nov 13, 2024

MrCreosote left a comment

MrCreosote Nov 14, 2024

MrCreosote commented Nov 14, 2024

MrCreosote left a comment

MrCreosote Nov 14, 2024

briehl Nov 19, 2024

MrCreosote Nov 14, 2024

MrCreosote Nov 14, 2024

briehl Nov 14, 2024

MrCreosote Nov 14, 2024

briehl Nov 19, 2024

briehl commented Nov 14, 2024

briehl commented Nov 14, 2024

MrCreosote Nov 14, 2024

briehl Nov 14, 2024

briehl Nov 14, 2024

MrCreosote Nov 14, 2024

MrCreosote Nov 19, 2024

MrCreosote Nov 19, 2024

briehl Nov 19, 2024

MrCreosote Nov 19, 2024

sonarcloud bot commented Nov 19, 2024

MrCreosote left a comment

MrCreosote Nov 19, 2024

		# Data Transfer Service Manifest format (which is a specific JSON format)
		DTS_MANIFEST = "JSON"

DTS manifest file part 1 #205

Are you sure you want to change the base?

DTS manifest file part 1 #205

Conversation

briehl commented Nov 13, 2024

MrCreosote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrCreosote commented Nov 14, 2024

MrCreosote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briehl commented Nov 14, 2024

briehl commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 19, 2024

Quality Gate passed

MrCreosote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment