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

Allow subdirectories for add & delete granules #20

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

ritvje
Copy link
Member

@ritvje ritvje commented Oct 3, 2023

Add a config option that allows keeping the subpath given in arguments when adding granules.

E.g. if config file has

geoserver_target_dir: /smartmet/radar/geotiff/radar-optclass/

if the filepath for add_granule.py is given as fivih_3067/20230926_1300_fivih_ppi-0.3_optclass.tif, the filepath is interpreted as

/smartmet/radar/geotiff/radar-optclass/fivih_3067/20230926_1300_fivih_ppi-0.3_optclass.tif

@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Merging #20 (2f8d905) into main (7721ac3) will increase coverage by 0.64%.
Report is 7 commits behind head on main.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
+ Coverage   93.51%   94.16%   +0.64%     
==========================================
  Files           4        4              
  Lines         833     1028     +195     
==========================================
+ Hits          779      968     +189     
- Misses         54       60       +6     
Flag Coverage Δ
unittests 94.16% <93.54%> (+0.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
georest/tests/test_geoserver.py 100.00% <100.00%> (ø)
georest/tests/test_utils.py 100.00% <100.00%> (ø)
georest/utils.py 95.65% <100.00%> (+0.65%) ⬆️
georest/__init__.py 83.85% <33.33%> (+1.92%) ⬆️

@ritvje ritvje requested a review from lahtinep October 3, 2023 06:38
Copy link
Contributor

@pnuu pnuu left a comment

Choose a reason for hiding this comment

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

This seems to include also the changes in #19 . Please include only the changes of the new feature so that the changes still needed for #19 are not needed to be added also in this one.

If we're defining multiple layers with the same config file,
it might make sense to have the files in subdirectories per layer
this allows keeping the subdir specified in argument
or keeping the path from the geoserver granule location

Note! Deleting files with this might fail if the path
in geoserver is different than path on host server!
@ritvje ritvje force-pushed the feature-subdirs-add-granule branch from 4b7a305 to acb190b Compare October 3, 2023 09:57
Copy link
Collaborator

@lahtinep lahtinep left a comment

Choose a reason for hiding this comment

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

Just a couple of comments that would simplify the configs a bit for cases where sub directories don't need to be handled.

I'll also push some additional tests for georest.utils.convert_file_path() to this branch in a moment when I've finalized them.

@@ -286,7 +286,7 @@ def add_file_to_mosaic(config, fname_in, filesystem='posix'):
This function wraps some boilerplate around adding a granule to a layer.

"""
fname = utils.convert_file_path(config, fname_in)
fname = utils.convert_file_path(config, fname_in, keep_subpath=config["keep_subpath"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could also be

Suggested change
fname = utils.convert_file_path(config, fname_in, keep_subpath=config["keep_subpath"])
fname = utils.convert_file_path(config, fname_in, keep_subpath=config.get("keep_subpath", False))

so forgetting the extra option from the config doesn't cause a crash.



def _delete_files_from_fs(config, gs_location):
delete_files = config.get("delete_files", False)
if not delete_files:
return
fs_path = utils.convert_file_path(config, gs_location, inverse=True)
fs_path = utils.convert_file_path(config, gs_location, inverse=True, keep_subpath=config["keep_subpath"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fs_path = utils.convert_file_path(config, gs_location, inverse=True, keep_subpath=config["keep_subpath"])
fs_path = utils.convert_file_path(config, gs_location, inverse=True, keep_subpath=config.get("keep_subpath", False))

@@ -85,9 +85,12 @@ def _write_property_zip(prop_paths, zip_path):
return zip_path


def convert_file_path(config, file_path, inverse=False):
def convert_file_path(config, file_path, inverse=False, keep_subpath=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently I haven't had a test case for this function, but we could add something now to test_utils.py. I'll write some and push them to this branch in a moment.

@lahtinep
Copy link
Collaborator

lahtinep commented Oct 3, 2023

@ritvje do you think the cases I added for keep_subpath=True tests for georest.utils.convert_file_path() make sense?

@ritvje
Copy link
Member Author

ritvje commented Oct 3, 2023

Looks good. Your version of the inverse option of georest.utils.convert_file_path() is much better, I was struggling a bit with that.

@ritvje ritvje force-pushed the feature-subdirs-add-granule branch from 47167d0 to 2f8d905 Compare October 3, 2023 13:02
Copy link
Collaborator

@lahtinep lahtinep left a comment

Choose a reason for hiding this comment

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

Looking good, merging.

@lahtinep lahtinep merged commit 261d19a into main Oct 4, 2023
8 checks passed
@lahtinep lahtinep deleted the feature-subdirs-add-granule branch October 4, 2023 06:07
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