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

Fix deploy script for Synology DSM (wrong RegEx for path extraction) #4810

Closed
wants to merge 2 commits into from

Conversation

Eagle3386
Copy link
Contributor

Fixes wrong RegEx as mentioned in corresponding deploy API issue.

Really don't know, how that'd slipped through, because I wrote the correct RegEx just a couple of comments above the linked one… 🙈😅

Anyway, this fixes the issue once & for all. 😎

@Eagle3386
Copy link
Contributor Author

@Neilpang I don't know why GitHub states "branch conflict" for simply changing the RegEx range from 0-9 to a-z.… 🤔

@winromulus
Copy link
Contributor

@Eagle3386 PR #4809 was already merged. Probably that's why you get conflicts.

@winromulus
Copy link
Contributor

@Eagle3386 considering Synology's naming conventions, I wouldn't assume that the entry will not contain any numeric or other characters in the path. Ideally this would have been picked up using jq instead of sed.

@Eagle3386
Copy link
Contributor Author

Eagle3386 commented Sep 27, 2023

@Eagle3386 PR #4809 was already merged. Probably that's why you get conflicts.

Probably not,because if it wasn't, my PR would still change the exact same RegEx range.
If it wasn't merged already that'd make sense, but it is, so…

@Eagle3386 considering Synology's naming conventions, I wouldn't assume that the entry will not contain any numeric or other characters in the path. Ideally this would have been picked up using jq instead of sed.

I've never come across an API endpoint of them that uses numbers. Other chars might be, but again, never seen that up to now.
Using jq would've been an option, yes - especially, since the file header even mentions it as a requirement.
However, neither the original script nor my update actually used it before & so I opted to follow the current way. That's because AFAIK jq isn't available by default on all platforms & Neil's requirement is to write cross platform compatible code, using "standard tools".

However, I'm fine with ^" as RegEx selector instead of my attempt with a-z., so closing this PR now.

@Eagle3386 Eagle3386 closed this Sep 27, 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.

3 participants