-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix birds not creating their wps output under each bird name #203
Merged
tlvu
merged 7 commits into
master
from
fix-birds-not-creating-their-wps_output-under-each-bird-name
Feb 17, 2023
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
2b7d8d9
finch, raven, flyingpigeon: output under wps_output under their respe…
tlvu 073ba02
finch, raven, flyingpigeon: create output dir under wps_output
tlvu 88cc8ee
finch, raven, flyingpigeon: match outputurl with outputpath
tlvu dad44c7
Merge branch 'master' into fix-birds-not-creating-their-wps_output-un…
Zeitsperre a026423
Merge remote-tracking branch 'origin/master' into fix-birds-not-creat…
tlvu dfc1d0d
CHANGES: Fix birds not creating their wps output under each bird name
tlvu dcbe4d8
Bump version: 1.23.1 → 1.23.2
tlvu File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
1.23.1 2023-02-13T18:31:02Z | ||
1.23.2 2023-02-17T02:54:37Z |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not so proud of this hack. Question for someone with more WPS config knowledge, is there an option in the config file to tell the server to create the outputpath dir if it does not exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we were to add this to the makefile for all birds instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised because the FileStorage code uses os.makedirs, which as far as I understand should create the missing path. https://github.com/geopython/pywps/blob/711219792be8b3d6a175a81152282dc5046d412b/pywps/inout/storage/file.py#L97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know where PyWPS fails if you don't create this directory ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh indeed ! Maybe the dir will be created on first use, not on bird startup so it's fine that's it it not there.
I saw this warning during bird startup
server->outputpath configuration value /data/wpsoutputs/finch is not directory
so I assumed too fast.Will retest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my end, I'm ok to wait for a PyWPS fix if that avoids throw-away code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dbyrns Unfortunately geoserver and postgres are not in a "component" layout. We have been adding new components using the new pluggable design but we have not migrate all the existing pieces to this pluggable design :(
@huard no big deal with throw-away code, it's just this section of
mkdir
that is throw-away. Waiting for PyWPS means not only waiting for a release of PyWPS but also the integration into our birds then a new build of all our birds.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, I'm not suggesting to make them proper components, but only to extend the pre/post compose loop to include the "built-in" components in the config directory. This way any existing services could include custom scripts.
This time I agree that we can wait for PyWPS, but we should keep that in mind if component related stuff are to be included in the
pavics-compose.sh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a quick win. But I'd say also a potential throwaway code if we migrate all existing components to the new pluggable design. Potential usecase for a complete pluggable design is not all organisations deploying PAVICS will want all the current components activated. Maybe they just want Thredds and their birds, not our birds.
Anyways, back to this PR, it's a cheap throwaway and a quick win. I'd rather take it now than wait. I can make the pre/post for
configs/
as well since we are into cheap throwaway for quick win. But I am fine if we prefer to wait.Note the other issue bird-house/finch#160 probably also need this same
mkdir
hack or the matching proper fix on PyWPS side. So either we solve both issues now or we wait for both issues.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@huard Are you okay to merge this quick hack for this fix? Not sure how much time I'll need to debug and perform the proper fix in PyWPS. All the old birds (hummingbird, ...) using the old "Buildout way" are basically doing this same hack, ie mkdir themselve outside of PyWPS.