PrivateBin
Replies: 4 comments 10 replies
-
|
Here my small IMHO / quickly written up, I dunno if this is all or so. Bug introductionFrom a cursory look:
Bug impact
Safety guards…we already have and why did they fail?:
|
Beta Was this translation helpful? Give feedback.
-
|
So first up, thanks for discussing this, because I felt a little bit left alone on it and hence also didn't want to rush it further. I appreciate any input. I'm still time-bound (as we all are, I assume) so I wont be in favor of any options that will add to the workload. It is also certainly not the first time we have a buggy release. We never retracted any of it, which is also why I never considered doing so this time. The most recent buggy release was 1.7.0, which I released a fix for in 1.7.1. We do as part of the release checklist have a step that asks to do a manual retesting of the key features. I usually spend about 5 minutes trying various things, changing options, add a discussion, comment a few times, also nested, add a burn-after-reading paste, etc.
It has been my understanding, that the slow release cycles we used to have were not well liked by both maintainers and users. With the automation we added around the release and not having to do big news blogs every time, it is much more manageable for me. I felt we also got a lot more engagement with our community, since we started doing it. On the topic of retracting: Can we please better define what exactly counts as such? Was the message added to the release page enough? How much further would we need to go (turn release back into draft, un-tag, rewoke published container images, revert release commits in those other repos, ...)? I fear that would quickly cause a lot of work, also to undue afterwards. Also, with container images it is super easy to revert to an older version. Using git a revert is also simple and the manual downgrade consists of unarchiving a second tar-ball and copy-paste the files a second time. Tedious, but not that difficult. Only the users with database backend couldn't easily go back, since we dropped that row. They would have had to restore a backup of the db. Slowing the release cycle down again: I don't think that would have prevented it either. Without a release no one tests it. If anything, I would in retrospect question if I should not have released the fix ASAP. Maybe next time we could discuss this up front, so no one feels stepped on their toes? Fixing forwards: With the quick releases it is certainly possible to also quickly release a fix, see 1.7.1, done the same night. This time, though I suspected what caused it, I needed to find time to sit down and work it through, also to find a solution that worked for bootstrap5 (and page, both use a straight select-form-element). I could have released this without asking for a review, last Thursday. I had banked on there being a review by Saturday, release it then. Of course I had also hoped to get to include some bootstrap5 improvements, which we now have included, but that should probably not have played a factor. Unit tests: This could have been detected by JS unit tests, but those are much weaker than our PHP ones and only cover about 62% of the logic, compared to 85% (most modules are above 95%, but the cloud storage backends are badly or not at all tested, dragging the score down). In particular, the problematic line is only covered one time and the bootstrap(3) specific event handler isn't covered at all. E2E tests: This 1.7.2 problem wasn't due to an interaction of front- and backend (server logic), but was specific to the frontend code (JS & DOM-tree). If we had JS unit tests covering both selection of expiration for bootstrap 3 & 5 style HTML we would have detected this. I'm not against them, as long as I don't have to maintain them. I'm not a fan of them because I won't be able to run them locally and offline. Meaning I can't do quick iterations. I would prefer if everyone in the team focused on increasing the JS coverage first, before we introduce yet more complexity. The 1.7.0 problem probably would have been caught in an E2E test, but I also still don't quite understand why it didn't show up in my release testing. I blame caches, but I don't know for sure. |
Beta Was this translation helpful? Give feedback.
-
|
On e2e tests:
Hmm AFAIK you can totally run them offline, depends on the setup etc. It would in an ideal world, just spin uop a backend and then run cypress (or so) starting a browser doing some test. But yeah maintenance is surely a key thing. Testing hiearchy usually tells: e2e tests are hard to write, but the tip that can cover a solid testing base. Also, I hope, once written it can just do the "release checklist manual testing" manually… Created #1335 for it now to have some actionable issue to track it and make people aware this is potentially something we want. Unit tests: Oh good point, then we have something to improve here on. Again an issue to track it more actionable: #1336 |
Beta Was this translation helpful? Give feedback.
-
|
Release cycles: Yeah I agree, doing fewer ones also does not help. Retracting: I am as unsure as you are. AFAIK in Docker you can just re-tag BTW the thing is less about reverting local installations (which is possible, but as you said may actually be not that easy in that example about DB backends – also note sysadmins do not necessarily know what DB changes we have done in a release, that is not [always] part of the Changelog), but about preventing new installations from upgrading to a potentially buggy release. |
Beta Was this translation helpful? Give feedback.
-
|
Here a suggestion for 5 whys on this… 5 whysPrivateBin v1.7.2 does always use 24h for expiration time.
|
Beta Was this translation helpful? Give feedback.
-
Not quite: I changed it because it didn't work with the bootstrap5 template - I then wondered how it could have worked with the page one? It turns out that some time years ago we changed the code to make it work (better?) for the bootstrap template, which made it not work for page and now bootstrap5. So in my eyes it is the bootstrap(3) template which is the odd-ball with the weird select + list menus. When we retire it, we can clean up the special bootstrap(3) code paths, but there should not be much left that is only there for the paste template. AFAIK there is only a few places we have a fallback for templates that have no bootstrap modals and do classic JS dialogs instead. |
Beta Was this translation helpful? Give feedback.
-
|
How do we get more maintainers onboarded? Clearly we need more eyeballs on the code. Also the release burden so far has been only on my shoulders. Now I know that one is a bit of a touchy subject, with the xz-incident recently, but in any case, we do need more developers participating, which can then become code-reviewers and start taking over maintainer-roles. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, at least we already have precautions against the xz incident (SLSA), … anyway, yeah, as for your question, I dunno. And I am also very grateful that you do the release stuff! 👍 |
Beta Was this translation helpful? Give feedback.
-
|
Hello, I think, in the hurry of bumping to v1.7.3, a break has been introduced in the The previous value, I also noticed that, if the app is in version other than the one on the linked line (like |
Beta Was this translation helpful? Give feedback.
-
Thank you, reverted in b96c8ae. The culprit is indeed an overzealous sed: Lines 31 to 38 in 1264418
The way the switch is supposed to work is that it is given the old database version (or 0.21 if the config table containing the database version doesn't yet exist). It then uses a switch with fall-through, so that it starts performing all upgrades from the earliest match. If one skips upgrades, like from 1.6.2 straight to 1.7.3, yes, it would never hit the 1.7.2 upgrade and you'd need to manually upgrade the schema. We could solve this by adding case statements for every version, or instead do repeated if-statements with a version compare greater-equal in each. I'll prepare a PR with the latter, for further discussion. Edit: see #1343 |
Beta Was this translation helpful? Give feedback.
-
|
How can we fix the sed command though? |
Beta Was this translation helpful? Give feedback.
-
|
I've no idea. We do want the sed to touch that file, so it can update the version number in the docblock header, but would not want it to update versions in the upgrade function. But we also want the sed to be able to replace multiple findings, for example in the Controller.php, where we want to also have it replace the VERSION constant. Any suggestions would be welcome. Only trail of thought I had is that I don't remember the purpose of having the version number in the docblocks in the first place? If we could drop it from there, there would be a lot less documents that needed to get touched by that sed. Templates, some Markdown docs, Controller.php, but none of the JS, CSS, etc. It would also eliminate the risk of forgetting to update the SRI-hashes in the templates because the version in privatebin.js got incremented. But I assume it serves a purpose to have the number in those files? |
Beta Was this translation helpful? Give feedback.
-
|
I doubt it's very useful. Even a header as part of #1261 who have big requirementson license stuff on that AFAIK you also just need a fixed license header. See https://git.fsfe.org/reuse/example/src/branch/main/src/main.c as an example, only the year potentially needs updates. So imho drop it. And then exclude the file or separate it into two see commands, where it is excluded. |
Beta Was this translation helpful? Give feedback.
-
|
Further research suggests that phpdoc doesn't even make use of it at the file level. It is intended to provide the API version at class or method level. So it was likely a misuse/misunderstood usage, probably introduced as part of the PSR-refactoring. See #1344 for a proposed cleanup. Happy to get rid of one more item that has caused me to do extra manual efforts during releases, before automating it with the Makefile. |
Beta Was this translation helpful? Give feedback.
-
See #1309 for the backstory. TL;DR is: v1.7.2 is a buggy release that slipped through. (IMHO likely because we have no kind/way of end-to-end testing our Frontend and Backend together,
but the cause and how to prevent the bug itself, may be a thing for another topic to discuss.) In any case, the thing was, we had a buggy release out there (including Docker images etc.) and needed to release a hotfix.This took some time (with all the reviews needed etc.) and people already upgraded and reported it in the issue and related ones – those, who have noticed the bug. Many probably did not do so. And as
The question is: How can we, in such a situation, – the next time – maybe better de-publish such a release? What should we do? What is a procedure?
What are the lessons learned? (Okay this would be more broad, dunno, if we want a different topic or also handle it here.)
What happened (timeline)
The issue itself
Faulty line with fix: https://github.com/PrivateBin/PrivateBin/pull/1322/files#diff-4c30f267da1a33c8d947890833316f48f2c58b89bc0ae567a469056ce98b96e5L4374 (in repository as permalink)
Working line before problem:
https://github.com/PrivateBin/PrivateBin/blame/08aa10a4b71b9aa9c837fa030f79de9ddb905203/js/privatebin.js#L4374
Beta Was this translation helpful? Give feedback.
All reactions