alastairp: what do you think is a good way to handle the above issue in the test.sh script. The test.sh script currently exits with a ) exit code even if the tests fails. I was thinking to use `set -e` but that doesn't work because we use functions that return non-zero values eg: `is_unit_db_running`. Other option is to store the exit code of the command that ran tests in test.sh and then return it at the end of the script.
This looks fine to me. Do you think there's a better a solution?
alastairp
_lucifer: hi, I saw that message last week
does test.sh run multiple tests in the same job? (e.g. frontend runs 3 things in a row, right?
so if the first one fails, will the other two run?
_lucifer
No, we call it thrice to do three different things.
and do we try and do cleanup at the end of the job?
_lucifer
test.sh handles that
alastairp
even if you set -e ?
_lucifer
actually set -e doesn't work here as i mentioned above. my intent was set -e then even if cleanup doesn't happen in the CI its fine because the container will just go away.
alastairp
yeah, right
_lucifer
when is_unit_db_running or any other function we define in the script returns a non zero value it exits.
so we'll have to handle it manually i think.
alastairp
I was going to suggest one option would be to use exec, that makes docker-compose take over the scrit, and its return value will be the return value
but that means you won't be able to do a cleanup after
_lucifer
that's an issue when we run locally.
alastairp
yeah
so, you were thinking of catching the return value of docker-compose, and then returning that at the very end of the script?
_lucifer
yes
one issue is that we lose the return value of the cleanup but i see we are not using it anyway, we just exit 0.
alastairp
I don't see a problem with not having the return value of the cleanup
_lucifer
cool, let's do that then catch the return value and return it at end.
alastairp
even though it's a bit annoying I still think it's the better way to go, using test.sh in the action
_lucifer
yeah, also we'll probably replace it with a test.py soon which should be easier to work with IMO.
outsidecontext
Nena should have sung about "100 Luftballons", then it wouldn't fool Picard's guess track number from filename :(
in get_or_create() I would like to set default mint_ts and max_ts timestamps for a user into redis, but only if the user is new. but in the db.user.py module is the wrong place to do it (logically, and also importing the timescale listenstore yields an import circular dependency. so it should be done in listenbrainz/webserver/login/provider.py
_lucifer
ruaok, python packages set up on new cluster. dataframes generated successfully. there is an incompatitble change wrt to schema in spark 3.* which needs to be fixed before recommendations start working. currently working on that.
ruaok
but, there I can't tell if the user was created or not -- could return a tuple of (user, created), but our tests call this function 94 times, which makes the PR much larger and more noise in our codebase.
thoughts for how to best to this iliekcomputers _lucifer alastairp ?
_lucifer: thanks. this is all perfect timing, I hope to get recommendations working again this week.
alastairp
ruaok: sorry, was in a call and didn't see your first question
ruaok
no worries.
alastairp
in django, their version of get_or_create returns a tuple (object, created), where you can check this
but as you point out, the method is already used in a bunch of places
ruaok
94 more changes to the PR would suck.
alastairp
oh, you already said that. missed that part of the line
ruaok
I could just add another get_by_mb_row_id call to check to see if the users exists. if it is fetched, the next call should effectively be cached in PG and return quickly.
_lucifer
if the only issue is that, we could do it in a separate PR and merge that first.
alastairp
just loading code up now
ruaok
_lucifer: yeah, I'd honestly prefer to do it in different way and not clutter up our tests more than needed.
alastairp
(there are also 34 instances of `create` in tests which for some reason don't use get_or_create :|)
ruaok: hmmm
ruaok
indeed.
alastairp
if you did it in provider.get_user, you could get, if it's none set a sentinel, do get or create, then if it was created, set the redis stuff
that's what you suggested?
ruaok
yeah
alastairp
yea, fine by me
ruaok
k, less mess.
alastairp
one potential problem there is that this is only used from the web login flow (right?). if we need this data in redis for an integration test, we might have to create it manually, which then means we might need 2 ways of making the same data
alastairp: I dont think we need an integration test -- I'll add a regular one and should be fine.
meh, we have no tests at all for testing the login process.
_lucifer
a good time to do that when we do account migration
ruaok
agreed.
alastairp
ruaok: right, I didn't specifically mean that we need an integration test for this. I mean, if you have an integration test that requires reading these values from redis, how are you going to set them?
ruaok
by calling the cache.put -- afterall I am only inserting (0,0)
the idea is this: if a user creates an account and then goes to their listens page, that would causes us to do an INDEX scan on the whole DB, with the guarntee of not finding anything.
but inserting (0,0) for the timestamps, means that the timescale listenstore knowns there are no listens and does not query.
alastairp
ok, good
_lucifer
these keys will not be expiring automatically?
CatQuest has quit
CatQuest joined the channel
CatQuest has quit
CatQuest joined the channel
ruaok
no, they are now intended to be in redis at all times without expiry. we now update the keys when the DB values change.
in fact this is the only reason why listen counts were ever accurate in the first place, lol. ;)
_lucifer
cool, you might want to pass `time=0` manually then
we'll be updating BU sometime soon so that the caller has to always specify a timeout. `time=0` if you do not want expiry.
ruaok
oh, that's important.
_lucifer
New recommendations generated.
ruaok
:D
alastairp
this is a good place to remind us that we had this open ticket about separating the redis interface in BU into cache (epheremal, may disappear) and data (need to keep around permanently), and if we should have 2 different interfaces for that task [not suggesting we do it now, let's just use cache, but something to keep in mind for future improvements]