lucifer: great, let me just push my changes now then
2022-06-07 15839, 2022
lucifer
alastairp: oh i just pushed, you may need to rebase again. sorry š
2022-06-07 15851, 2022
alastairp
no worries, my changes are pretty small
2022-06-07 15854, 2022
lucifer
ah cool
2022-06-07 15859, 2022
Sophist_UK has quit
2022-06-07 15843, 2022
lucifer
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.
2022-06-07 15858, 2022
lucifer
(this is LB stats fwiw but i thikn similar considerations applies to CB)
2022-06-07 15813, 2022
Sophist-UK joined the channel
2022-06-07 15826, 2022
lucifer
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.
2022-06-07 15830, 2022
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
2022-06-07 15849, 2022
alastairp
right, and remaining transitive dependencies could just be "didn't realise that it wasn't an explicit dependency"
2022-06-07 15815, 2022
lucifer
i see. i was trying `pipdate` to update all versions and then read the major version changelogs to revert breaking ones.
2022-06-07 15800, 2022
lucifer
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`.
2022-06-07 15808, 2022
Sophist_UK has quit
2022-06-07 15823, 2022
alastairp
lucifer: in lb?
2022-06-07 15826, 2022
lucifer
CB
2022-06-07 15858, 2022
lucifer
once you push the redirect changes to CB PR, i'll make same changes to LB PR.
2022-06-07 15835, 2022
alastairp
I was able to fix the dependency lookup by just removing coverage - didn't have to add any other transitive dependencies
2022-06-07 15843, 2022
Sophist-UK joined the channel
2022-06-07 15854, 2022
lucifer
oh i see
2022-06-07 15818, 2022
lucifer
i'll revert that commit then.
2022-06-07 15838, 2022
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
2022-06-07 15847, 2022
alastairp
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
2022-06-07 15809, 2022
alastairp
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
2022-06-07 15850, 2022
alastairp
however, flask2 doesn't appear to do this behaviour (which is why the tests are failing)
2022-06-07 15805, 2022
alastairp
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.
2022-06-07 15838, 2022
lucifer
maybe copy over the test case to BU and add the relevant fixes and use that in our tests or maybe something else.
2022-06-07 15847, 2022
alastairp
right, we can always just check that response.location.endsWith("/our/url")
2022-06-07 15848, 2022
alastairp
that's fine
2022-06-07 15812, 2022
alastairp
the docs in the flask website are kind of testing something a bit different
2022-06-07 15812, 2022
lucifer
makes sense. let's do that.
2022-06-07 15823, 2022
alastairp
parsing the url and then checking .path
2022-06-07 15850, 2022
alastairp
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
2022-06-07 15800, 2022
alastairp
double escape?
2022-06-07 15804, 2022
alastairp
copy_from
2022-06-07 15816, 2022
lucifer
oh i had just fixed that.
2022-06-07 15824, 2022
lucifer
not sure why it errored again
2022-06-07 15827, 2022
lucifer
ah i missed the copy_from one. only did copy_to's
2022-06-07 15807, 2022
lucifer
ok fixed now. if tests pass, i'll squash all copy_{to, from, expert} commits into 1.
2022-06-07 15826, 2022
alastairp
ok great, if we can keep my redirect one there in the middle somehow then that'd be great
2022-06-07 15830, 2022
alastairp
but otherwise looks good to me
2022-06-07 15850, 2022
lucifer
yup i'll do an interactive rebase, and move around the commits to keep the redirect one separate
2022-06-07 15808, 2022
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.
2022-06-07 15818, 2022
reosarevok
Thanks
2022-06-07 15839, 2022
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
2022-06-07 15803, 2022
alastairp
lucifer: ok thanks, let's merge CB and release to beta and see what happens?
2022-06-07 15808, 2022
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?
2022-06-07 15814, 2022
lucifer
alastairp: yes sure
2022-06-07 15852, 2022
alastairp
I don't follow too well, can you show some code examples? is this an if/else for different functionality in a test?
2022-06-07 15802, 2022
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.
2022-06-07 15818, 2022
alastairp
or that in some instances the data is a column and in some it's a function?
