#metabrainz

mayhem: i made a few changes to the cache to make it work and left it running overnight on wolf. going fine so far.

Great! Still using Spotipy?

mayhem: sort of, i forked it and edited it to accept a custom retry strategy.

alastairp: the dumps you mentioned yesterday, do those include mb_metadata_cache table?

mayhem: Wonder if we're listed somewhere in a list as hiring or some weird shit

Given we keep getting hail mary job proposals

I have no idea, reosarevok . Just keep ignoring them.

lucifer: oh, good idea!

Yeah, I guess. I considered answering to one and asking where they got the idea of writing to us

But they might get encouraged :p

morning lucifer, actually - good point

looking back at my notes, we weren't planning on including it, but perhaps it's a good addition

[critiquebrainz] 14anshg1214 opened pull request #456 (03master…handle_deleted_and_redirected_entities): Handle Deleted and Redirected Edition Groups https://github.com/metabrainz/critiquebrainz/pu...

zas: well, it looks to be working, but doesn't seem aggressive enough with blocking the SSH brute force attacks

hmm, perhaps I'm missing something

alastairp: If you have some time can you please review CB#452

Allow CB to review Literary Works: https://github.com/metabrainz/critiquebrainz/pu...

ansh: I'm doing that right now!

Thanks

atj: it would be convenient to run "cscli completion bash | sudo tee /etc/bash_completion.d/cscli" after installation

zas: I'll update the role

atj: one package is missing on crowdsec machine: crowdsec-firewall-bouncer-iptables (not installed), I think it is why it doesn't block ssh scans atm

it is installed on rex

I did that intentionally, just so if something went wrong we weren't locked out of the API server ;)

we can remove it once we're happy everything is working as expected

atj: for extra safety, we should add a whitelist (on all nodes) including our IPs -> https://docs.crowdsec.net/docs/whitelist/create/

I'm a bit disappointed we cannot control whitelisting via LAPI

ansh: can BB have recursive redirects?

Yes

For example you can check this entity id `dd40b465-931f-46ee-b2ae-28685b19f8d8`

interesting. it might be worth discussing the reasoning for this with monkey. it might make a bunch of things easier if we convince him to change this

I don't think that's going to change. The idea is to be able to go back in time over all edits, including merges

oh right, you have this time travel thing

Yep, exactly. We don't want to lose the ability to revert potentially destructive edits

ansh: we'll have to check this query then, to ensure that it stays fast even if the table gets much larger

alastairp: If this takes more time, I'll then switch to the same way BB does it

ansh: ah, I see

I did an explain plan on it:

I'm a bit worried about the sequential scans, although on a table with only 600 rows this isn't going to be a problem. the question is what happens when it has 60,000

in fact, I see that there isn't actually an index on `source_id` either, so monkey's query is likely to get slower and slower as more rows appear, too

I suspect so too

monkey: if you want to open a ticket for it, we can sit down and test this after holidays

With pleasure

zas: I added functionality to manage a custom whitelist to the role - https://github.com/metabrainz/ansible-role-crow...

do you want to include our personal IPs and/or server IPs?

alastairp: great, i suspect the cache table to be smaller than the entire MB release dump to import into spark and easier to work with as well.

oh, perfect. if it's needed for spark then we definitely make it

alastairp: BB-684

BB-684: Investigate entity_redirect table SQL queries https://tickets.metabrainz.org/browse/BB-684

mayhem: i'll need to clean up the the branch a bit before puting it on gaga. should i migrate this to PG as well while at it or let it use couchdb for now?

[bookbrainz-site] 14MonkeyDo merged pull request #867 (03master…fix/edition-group-ac): Fix(AC): Handle AC modal state for editionGroup entity https://github.com/metabrainz/bookbrainz-site/p...

Helloo everyone!! ヾ(≧▽≦*)o

aerozol: are the new designs good ??

akshaaatt what do you say??

atj: server IPs

also akshaaatt now I will be implementing repositories for playlists , albums, artists and song entities

Kinda got late coz internship season has begun here in my college and I was a bit busy brushing data structures and algo and other subjects.

But will soon speed up contributions ヾ(•ω•`)o

alastairp: uh, i encountered a snag while updating flask tests. https://github.com/metabrainz/listenbrainz-serv...

i had forgotten that the connection is scoped to a request so it'd get cleaned up after the first request.

i was able to work around it this way: 1) instruct teardown to not do anything in TESTING. 2) push a manual app context in each test so that we can patch db_conn for the test. means we'd have to write the entire test in a with:.

not ideal but it seems to work.

Yes yellowhatpro, I am pretty much aligned with the designs.

alastairp: oh i forgot to mention, another fun fact i learned. in postgres, value of `now()` is fixed for the entire duration of the transaction and equal to the time when the transaction began! if you want actual clock time, you need to use `clock_timestamp()`. i had to patch this at multiple places to make db tests work in LB.

lucifer: oh yeah, I knew that about now(), but in most cases I guess it was ok?

regarding db connections... so by scoping to a request we are still only creating one connection per request in the main webserver (as opposed to one per db query?)

but this causes problems with tests or testcases that include multiple requests?

ah right - because you need a single connection handle in the test case to be able to roll it back, and if you close it at the end of a request half way through a test then it's no longer valid?

alastairp: there are multiple places where we order by created which is set to now() and the ordering of results turned out to be wrong. also some places we check < now() so results were incomplete so on.

yes to the reason for needing a single connection.

requiring us to push an app context inside every test method seems like a recipe for disaster, unfortunately

lucifer: didn't we do some stuff in BU tests with a pytest context? I wonder if there are some pytest fixture behaviours which we could use to set up/tear down the connections?

and I see about the now() - because we no longer have a separate transaction per db method I can see how this is going to cause different results

ansh: reviewed works, it looks pretty good. just a few things to fix up

alastairp: iiuc, that had to with not using unittest.TestCase. if we were using that in BU, we could have just used a setUp method or setUpClass etc.

or if we had to reuse a fixture across TestCase classes.

i tried to do a push in setUp and pop in teardown instead of using with app.app_context():, which is essentially what the with does but it fails that way do to some reason.

i wonder if that has to do with how contextvars work...

mm, right. that would have been my preference too. odd that it didn't work

i'll try to get that working otherwise we can look at this together again after holidays.

[bookbrainz-site] 14MonkeyDo opened pull request #868 (03master…fix-ac-link): Fix(AC): Fix author credit link https://github.com/metabrainz/bookbrainz-site/p...

atj: I added a config to nginx on crowdsec to test prometheus endpoint. That's not ideal, because it is expected each node running crowdsec exposes its own endpoint.

atj: dashboard requires docker, see https://docs.crowdsec.net/docs/observability/da...

thanks for finding that, didn't look at that section of the docs

not sure it's worth the effort to automate that step with ansible

let's first test this dashboard, we need to add docker to the VM (via ansible)

I'll update the PR

done

all the API communication seems to be under the "/v1" path, so we can probably use the same virtualhost for the dashboard and the API

[bookbrainz-site] 14MonkeyDo merged pull request #868 (03master…fix-ac-link): Fix(AC): Fix author credit link https://github.com/metabrainz/bookbrainz-site/p...

alastairp: Thanks for the review. I've made the required changes

aerozol: The newest mockup on CB-442 (V2d) looks great! IMO that's a great improvement over the existing pages; we could fiddle with details for months but I think implementing it as is would already be a big win

CB-442: Improve layout of CB entity page https://tickets.metabrainz.org/browse/CB-442

alastairp: oh well, it turns out i was doing something stupid. i was doing `self.app.app_context().push()` and `self.app.app_context().pop()`. and .app_context() creates a new ctx so the invalid ctx was popped and it failed.

this works

lucifer: oh, great!

monkey: how do you think we should go about this redesign, then? something that we should start on now? Or wait for design system, or include into a react rewrite, or... ?

Currently the pages are not react-based? Are they jinja templates?

100% Jinja

We could just implement them with jinja template. Most of it will be css styles I reckon

and tbh, probably keen on keeping it like that for the moment

(Except maybe the filtering stuff)

monkey: cocinista order shipped already!

I don't think anything much on this page requires the design system. On the contrary maybe some elements designed here could end up making their way to the DS in the end

ok, great

monkey: oh excellent! I'm happy with them too 🎉

I've just asked sound.and.vision to have a look as he's the most active CB user, see if he can spot any opportunities for improvement

yellowhatpro: I like it! The dark is super easy on the eye holes 👁️

Quite different to the other pages though isn't it - so if akshaaatt has changes to make it fit in more with his other designs then go for it

monkey: do you want a mobile mockup?

That would most like ly be useful yes