yeah, that paco_mb.sh looks unusual to me. it should probably be removed but best to wait for zas to confirm.
zas
oh, it shouldn't have been committed
djinni`_ has quit
removed
that said, it shouldn't create problems, what if we want to add extra scripts (like a common script to be included in others) in this directory? Which script is relying on all scripts on this structure?
[listenbrainz-server] 14amCap1712 merged pull request #1676 (03master…invalid-listen-count): LB-982: User page does not load because of borked listen counts https://github.com/metabrainz/listenbrainz-serv...
on 1652, the duplication of the user_name/user_id is part of maintaining backward compatibility?
lucifer
yes
ruaok
backward compatibility with... what? people who have the page open and are receiving info?
monkey
Moin!
ruaok
moooin!
lucifer
there are two issues: 1) The normal listens have a user_name field in each item in listens array. in the PR, I have added "user_name" field to the item inside the array as well for consistency with the normal listens. the rationale is that this make frontend code less error prone, we have had a couple of bugs in the last few weeks around playing now listens which were caused due to missing fields (either due to ws or such stuff).
monkey
I have a quick fix I need to make a PR for
lucifer
the outer "user_id" field is not removed because of backward compat
ruaok
backwards compat with what?
lucifer
the endpoint already used to send that field so some client might be depending on it being present there.
ruaok
a websockets client or a random API client?
lucifer
api client.
sorry the PR is misnamed, its not about only WS.
i'll split it into two: one for api change and another WS change.
ruaok
don't worry about it. its fine.
I'm just trying to understand why we really need the duplicate key there at all.
if it is only in websocket responses, then the only backward compatibility will be with users who have the page open right now, right?
lucifer
yes right.
ruaok
but those will be broken by updating the container, so users will need to reload, which then means all clients have been updated.
which means we dont need the duplicate.
lucifer
right.
ruaok
(other than for consistency sake)
lucifer
the issue is this in api response too.
ruaok
yes, understaood. those will be harder to clean up and that is not the point of this PR.
I guess my question is this: if there is no real need for backward compatibility, why add a duplicate key for consistency, when we should be cleaning up the API endpoint?
perhaps we should decide what we what things to look like in the future and aim for that, rather than consistency that is duplicative.
ruaok is really not trying to be a pain in theass
lucifer
makes sense. hah don't worry. this is what i want to get the advice on, consistency now and cleanup at once later or clean what we can now and leave some inconsistency.
reosarevok
zas, lucifer: thanks, worked fine now :)
ruaok
My take: design for the future and aim for consistency for that. accepted some inconsistencies for the moment.
reosarevok
zas: if the scripts shouldn't assume those things, then we should mark stuff in some way, because the update_containers script we use for deploying MB also depends on docker-server-configs
yvanzo would know more since IIRC he wrote it
lucifer
sounds good.
zas
reosarevok: I'm fixing the script, in progress
Etua has quit
ruaok
lucifer: does that mean you adjust the PR a bit, then we merge and release?
I'd like to merge this PR before the release lucifer :)
lucifer
sure, no hurry.
reviewing it.
monkey
Glaring mistakes in a previous PR of mine…
reosarevok
yvanzo: anything for this musicbrainz-docker release?
lucifer
monkey: approved.
monkey
🎉
lucifer
i see they made MessageEvent generic in ts 4 so that might help in future.
monkey
OK, let's merge this quick before everyone realizes it's broken 😅
Yeah, definitely need typing on that, but couldn't figure it out at the time
yvanzo
zas: For two years, MB deployment scripts retrieve hosts/containers by listing/parsing files under "scripts/nodes/". Before that, it was hard-coded in MBS repo which had to be synchronized with docker-server-configs repo. We can document the current structure in a README.md file and stick to it, or have another (duplicate) structure dedicated to that.
yvanzo: I'm fine with such structure, but yes, we need to document it, and do not assume all scripts match actual servers. I'm working on it right now. I guess we could have a function to provide a clean list
reosarevok
Nothing new after that?
yvanzo
“common script to be included in others” can go in a separate directory.
zas: See "scripts/list_nodes.sh" (in docker-server-configs)
reosarevok
yvanzo: when you have some time, please check the blog draft too :)
yvanzo
reosarevok: yes, it isn’t complete yet.
monkey
> listen count < 0, oh yeah that's possible. Wonder how? So did I on a Sunday afternoon. Let's see how this happens.
:heart:
zas
yvanzo: yes, I saw, but none of those scripts checks if the node actually exists (for example, we still have boingo.sh). And this last one, assume start scripts start with "start_" (well, they do, but did we ever actually document that?)
(seeing you all work hard on pull requests and the like for release today)
ruaok
grrrr, docker still hangs on my tests. whyyyyy?
ok, its hangs on bono too.
lucifer: when you're done with the release could you please try to run `./test.sh listenbrainz/webserver/views/test/test_color.py` on the latest huesound branch?
monkey
Developers: "whew ! it's also broken elsewhere, I feel so relieved !"
