lucifer: great, let me just push my changes now then
lucifer
alastairp: oh i just pushed, you may need to rebase again. sorry 😓
alastairp
no worries, my changes are pretty small
lucifer
ah cool
Sophist_UK has quit
alastairp: do you know how the contents of the current requirements.txt file were choosen. for instance pip freeze lists 130 deps but the file has only 54 entries.
(this is LB stats fwiw but i thikn similar considerations applies to CB)
Sophist-UK joined the channel
at first it seemed to me the one in the file could be direct deps only we use but it seems there are some transitive ones too there.
alastairp
no, I can't remember. it's possible that I wanted to try and separate top level dependencies from transitive dependencies and so removed all of them? something like poetry would work much better here instead though
right, and remaining transitive dependencies could just be "didn't realise that it wasn't an explicit dependency"
lucifer
i see. i was trying `pipdate` to update all versions and then read the major version changelogs to revert breaking ones.
and doing pip freeze after that generated a much larger file so asked.
alastairp: that hour long test run which never completed was because pip was unable to decide which versions of transitive deps to use. so i pinned the latest versions of the needed ones. also, fixed an issue related to psycopg2's `copy_to`.
Sophist_UK has quit
alastairp
lucifer: in lb?
lucifer
CB
once you push the redirect changes to CB PR, i'll make same changes to LB PR.
alastairp
I was able to fix the dependency lookup by just removing coverage - didn't have to add any other transitive dependencies
Sophist-UK joined the channel
lucifer
oh i see
i'll revert that commit then.
alastairp
copy_to -> copy_exoert is a psycopg2 thing, or sqlalchemy?
lucifer: I've been digging into flask/werkzeug redirects along with the version upgrade, and I'm not sure it's worth trying to work out what's going on
from what I can tell: flask 1 uses werkzeug's redirect() method directly. if you pass it a relative url (e.g. /artist/foo) then at some point in the chain, it adds http://localhost in front of the url, this seems to happen really late in the pipeline, I can't work out where it happens
this is why flask_testing automatically adds this to the expected url in assertRedirects if it's not set, it must have been reproducing this behaviour
however, flask2 doesn't appear to do this behaviour (which is why the tests are failing)
but what I can't find, is when in flask or werkzeug they decided to change this behaviour
let's fix the tests manually for now and later look into replacing flask-testing.
maybe copy over the test case to BU and add the relevant fixes and use that in our tests or maybe something else.
alastairp
right, we can always just check that response.location.endsWith("/our/url")
that's fine
the docs in the flask website are kind of testing something a bit different
lucifer
makes sense. let's do that.
alastairp
parsing the url and then checking .path
right, but what I'm unsure about is why did flask decide to add a protocol/host to this stuff in the first place, and then why did they remove it in flask 2
lucifer: cool, thanks for helping to dig through that history. now that I'm aware of where/how the change was introduced, I'm happy to just update the asserts to check them being relative
psycopg2.errors.UndefinedTable: relation ""user"" does not exist
double escape?
copy_from
lucifer
oh i had just fixed that.
not sure why it errored again
ah i missed the copy_from one. only did copy_to's
ok fixed now. if tests pass, i'll squash all copy_{to, from, expert} commits into 1.
alastairp
ok great, if we can keep my redirect one there in the middle somehow then that'd be great
but otherwise looks good to me
lucifer
yup i'll do an interactive rebase, and move around the commits to keep the redirect one separate
reosarevok
yvanzo: huh, sentry has a ton of DBI connect errors from 6 h ago, but telegram says nothing. Any idea what that might be about? Unless it was about sir
reosarevok: Updated MBS/MBVM releases in Jira too.
reosarevok
Thanks
alastairp
yvanzo: reviewed. I think that as this is just guidelines for the docs it's probably a good idea to start adding docs based on it and see if we need to add additional guidelines
lucifer: ok thanks, let's merge CB and release to beta and see what happens?
lucifer
alastairp: lb fixes for copy expert are a bit complex to do correctly due to schema qualified table names, function calls (to_timestamp(0)) and literals ('null'). we can either add if/else check for 2 cases or manually change each entry to the correct escaping type. any preference?
alastairp: yes sure
alastairp
I don't follow too well, can you show some code examples? is this an if/else for different functionality in a test?
lucifer
alternatively, we can skip all escaping. we do know its not needed anywhere because we control column and table names. only that this is disccouraged.
alastairp
or that in some instances the data is a column and in some it's a function?
