-
Notifications
You must be signed in to change notification settings - Fork 10
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
Dashboard queries - including folder organization #925
Conversation
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'd like to split this into one PR for/
path support and one for each query.
@@ -77,7 +77,8 @@ async function run(request, context) { | |||
params.GOOGLE_PRIVATE_KEY = context.env.GOOGLE_PRIVATE_KEY; | |||
params.GOOGLE_PROJECT_ID = context.env.GOOGLE_PROJECT_ID; | |||
|
|||
return runExec(params, pathname.split('/').pop(), context.log); | |||
// take final URL path segment and then substitute @ with / to allow for /queries subfolders | |||
return runExec(params, pathname.split('/').pop().replaceAll('@', '/'), context.log); |
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.
Can't we just keep /
as the folder separator?
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.
The issue is that .pop()
takes only the last path segment (the file) and we lose the folder name to look it up later at https://github.com/adobe/helix-run-query/blob/main/src/util.js#L22. And taking the last two segments is invalid since not all have a subfolder under queries. And removing the first N path segments is invalid because sometimes /helix-services/run-query@ci####
(2 segments) is used while other times /helix-services/run-query/ci####
(3 segments) is used. After several iterations, it became a question of finding an approach that would always work.
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.
Look for the @
then split at /
and drop the first element.
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.
That will work for most but not all patterns (see the 3 segment example above). But your point is clear and this is a solvable problem. Use / as the path separator. I will close this PR without merging and open more atomic ones for each feature/query.
Added a few queries but more generally useful is the addition of folders under /src/queries. Use folder@file notation in the URL, for example /helix-services/run-query@ci5338/dash@pageviews?
Related Issues
#911