Repository navigation
Detect new screenshots - #696
Conversation
|
It finds now 3 screenshot(s) when it's based on #695 |
5fa2c2f to
6c52c7a
Compare
goneng
left a comment
goneng
left a comment
There was a problem hiding this comment.
Nice catch on the error-screenshot loop. find .. | while read ran the body in a subshell, so every COUNTER++ and body=... inside it was thrown away and error screenshots could never reach the PR comment. Moving to done <<< "$errorFiles" fixes that, and it retires the if [[ ${f} == "../view-op-error-*.png" ]] guard that could never match.
A few things I noticed, all small:
- Basename collisions.
newNameuses only$baseName, so two new PNGs with the same name in different directories overwrite each other, first viacpand then on the upload host. #695 goes to some trouble to disambiguate repeated attachment names, so it would be a shame to lose it one level up. - Scope of the search.
git ls-files --othersis repo-wide, and the build and result directories in the exclude list are normally gitignored, so--exclude-standardalready drops them. What is left uncovered is a stray PNG someone happens to leave in their tree, which would get uploaded and embedded in the comment. Scoping to the screenshot output directory might be tighter than excluding by pattern. echo "request_cmd=$request_cmd". This one is pre-existing rather than yours, but the new block copies it, and sincerequest_cmdis built with double quotes it carries the expanded-u $SCREENSHOT_USER:$SCREENSHOT_PASSWORD. Actions masks it in CI, so it only really shows when running locally. Worth mentioning because it bypasses theDEBUG_INFOguard just above it. Related:echo $$request_cmdwas printing the PID, so the old line happened to leak nothing, whereas the corrected one will.- Ordering. Does this want #695 in first? The new search looks for untracked PNGs, so whether it finds anything depends on where the extractor writes.
One request: since the change itself lives in hannesa2/screenShotScript, a compare link in the description would help, as the diff here is a single submodule line.
|
Withdrawing my fourth point, about ordering. Your comment on #695 answers it, and I had the dependency backwards: this PR is what makes #695's new screenshots visible in the first place, not the other way round. Now that I follow the mechanism: the comparison stages the base screenshots as a throwaway commit and relies on the case-insensitive runner filesystem to have the PR's extraction overwrite them, so So this can go in first, and a re-run of #695 should then actually show those three. The other three points stand. |
6c52c7a to
3648f3c
Compare
|
I applied your suggestion, I hope it's now more clear |
|
Much clearer, thank you. All three are addressed, and the run on #695 shows the collision fix working: the uploaded names went from Two tiny things, neither worth holding this up:
Looks good to me. Happy to rebase my pull requests onto master once this lands, as you suggested. |
3648f3c to
686e721
Compare
|
This make sense, I applied the suggestions |
Go ahead ! |
It's a follow up of #695 where CI doesn't detect new screenshots.
The work is done here hannesa2/screenShotScript#2 in screenshotScript