Just now I merged a PR which added many packages to the environment
#dev-contrib
tawdry vapor
tawdry vapor
It previously didn't have any packages besides the ones needed to run snekbox (falcon, gunicorn, jsonschema)
And they are all compatible with 3.8
mellow hare
Ohhhh, you know what, I might be thinking of the internal eval
green oriole
@woeful thorn I left the issue open to act like an issue for suggesting new package, should we keep that one open or make a new issue, perhaps on meta?
Also, hemm, do the bot need to be re-deployed? Because it is clearly not working..
woeful thorn
If someone wants to suggest a package they can open an issue for it
Monolithic meta issues get noisy and difficult to follow
Because it is clearly not working
Can you be more specific?
green oriole
green oriole
Maybe we should ask Joseph, he made it to work on his local bot
crude gyro
he was probably running snekbox locally
I'm at a computer now so I can check
when did we merge the PR for snekbox?
bout an hour ago?
hmm
looks like it has been deployed. the container is 1 hour old.
green oriole
I don't know how your stack works, but can it be because the bot need to be reboot? But I don't think it would make too much sense actually
crude gyro
nope, bot just sends an API request
makes no sense
I'm investigating btw
the container has the pipfile with all the stuff you added.
molten bough
Could that be because the sandbox doesn't allow any file handles, or can you read from the virtualenv?
green oriole
If you open a shell inside the container, can you access the libs?
crude gyro
yeah I'm trying to figure that out, but I'm not sure where the venv is
green oriole
@molten bough Joseph made it to work
crude gyro
looking at CI logs to figure it out now
green oriole
Im scared that it would not work anymore because we updated the image
crude gyro
doesn't make much sense considering the error
green oriole
And the new one doesn't allow access from the libs installed by pipenv
crude gyro
frankly, we should probably just make pipenv install them in the system python?
god knows it already has enough isolation
green oriole
It is not doing pipenv install atm?
molten bough
It's worth a try, I guess
crude gyro
RUN if [ -n "${DEV}" ]; then pipenv sync --dev; else pipenv sync; fi
it's doing this
which would build a venv
where do those venvs go by default?
green oriole
You can't just do pipenv run python3?
molten bough
They go in ~/.virtualenvs iirc
crude gyro
yeah, so
the venv just has the stuff needed to run the web server as far as I can see
/snekbox # pipenv run python -m pip freeze
attrs==19.3.0
falcon==2.0.0
gunicorn==19.10.0
jsonschema==3.2.0
pyrsistent==0.15.6
six==1.13.0
but there may be even more layers?
the system python doesn't have any packages either
it's a bit weird because the Pipfile here definitely has more than this
oooh
hang on
maybe someone forgot to lock
molten bough
Is the Dockerfile correct? Maybe they're not in the container
crude gyro
it's weird because there are some packages in this venv
just not the ones in the Pipfile
molten bough
Yeah, that is pretty weird
crude gyro
I can try to resolve it or something
I think that's a pipenv thing
aha
yeah okay
pipenv sync gives a lot of errors
many compiler-related errors still
molten bough
That'd do it
Is this a case of azure not realising the container failed to build again?
green oriole
The previous build have failed because of a missing compiler though
crude gyro
green oriole
And I thought Mark have tested it
molten bough
Is build-essential not installed? That's strange
crude gyro
well, let's see
crude gyro
lol, never mind
/snekbox # dpkg -l
/bin/sh: dpkg: not found
/snekbox # sudo apt install dpkg
/bin/sh: sudo: not found
/snekbox # apt install dpkg
/bin/sh: apt: not found
the fuck is this
is it because I'm using sh?
molten bough
No
crude gyro
it didn't seem to have bash
molten bough
Is apk there?
molten bough
You're on alpine
molten bough
That's an alpine container my dude
crude gyro
yep, this must be an alpine container
green oriole
We just switched my dude
crude gyro
haha, apparently we didn't.
tawdry vapor
the ci didnt build the containers
crude gyro
so that's interesting.
molten bough
Yeah, I suspect it didn't build the container
tawdry vapor
I think I spotted an issue like this when working on a PR and I was completely stumped by it
i.e. git diff saying there are no changes in dockerfiles when there clearly fucking are
molten bough
You didn't ignore them by accident?
tawdry vapor
no
crude gyro
@tawdry vapor you sound like you know what the problem with the CI is, then?
tawdry vapor
Yes and no
woeful thorn
Maybe let him think for a minute lol
tawdry vapor
It's just cause of this Retrieving the latest successful build using https://dev.azure.com/python-discord/96ac3876-1f84-4571-935f-c5bd3862b804/_apis/build/builds?queryOrder=finishTimeDescending&resultFilter=succeeded&$top=1&repositoryType=GitHub&repositoryId=python-discord/snekbox&api-version=5.0&branchName=refs/heads/master Comparing HEAD (4d41ba2bbcde47d1304d25c53953e5de62c24d64) against a4a8805587dc8e011fc2aa4276cc71631c58702a. No changes detected in docker/base.Dockerfile. No changes detected in docker/venv.Dockerfile or the Pipfiles.
But I don't know why it thinks there are no changes between those commits
if you open them for yourself you can clearly see there is a diff for the docker files
Is snekbox broken right now? Or does basic eval still work
crude gyro
!eval print("well, let's see")
stable mountainBOT
@crude gyro :white_check_mark: Your eval job has completed with return code 0.
well, let's see
ok so this isn't critical
green oriole
So who suck the most, git, or azure?
tawdry vapor
if git diff --quiet "${prev_commit}" -- docker/base.Dockerfile; then
echo "No changes detected in docker/base.Dockerfile."
echo "##vso[task.setvariable variable=BASE_CHANGED;isOutput=true]False"
else
# Always rebuild the venv if the base changes.
exit 0
fi```how much logging do we have from that
tawdry vapor
not enough
crude gyro
wait, I'm confused
where exactly did you push this base dockerfile change
I'm not seeing it on master?
oh wait no never mind
okay
tawdry vapor
git checkout 4d41ba2bbcde47d1304d25c53953e5de62c24d64
git diff --quiet a4a8805587dc8e011fc2aa4276cc71631c58702a -- docker/base.Dockerfile
echo "$?"
return code 1 means there is a diff
return code 0 means no diff
this returns 1 for me
and this should be exactly what CI is doing
green oriole
Just ran git diff --quiet a4a88055 -- docker/base.Dockerfile, in fact, does produce a non-zero exit code
OK well, mark is faster
Can we actually log an hash of the dockerfile or something, to make sure the ci got the updated version, and it is not ignoring it or something
tawdry vapor
I suppose
but it logs the commit hash
there is no reason it should have a different file
green oriole
Can it ignore it if for some reason git want to merge the file or whatsoever?
There is certainly a problem with the I not getting an updated version of the dockerfile
molten bough
it's not a case of the conditional being bad either
[gdude@trixie snekbox]$ if git diff --quiet "$(git rev-parse HEAD)" docker/base.Dockerfile; then echo "yes"; else echo "no"; fi
yes
It does work sometimes
I encountered this months ago with a PR
and couldn't reproduce
It started working after the first time
It may be an issue with the git index
Is there something I can run to ensure that the previous commit exists
this is what CI does already git remote add origin https://github.com/python-discord/snekbox git config gc.auto 0 git config --get-all http.https://github.com/python-discord/snekbox.extraheader git config --get-all http.proxy git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules origin
green oriole
I just ran those command, and in fact git does not see any difference
Oh wait, I'm silly, everything is fine
crude gyro
hmm
hey @tawdry vapor
why is this result False
because false doesn't equal true
I guess
so the variables aren't true. and then that's because of the docker check script?
crude gyro
gotcha
tawdry vapor
it thinks the docker files have not changed, so it doesnt bother building them again
crude gyro
I don't think I ever took the time to fully comprehend this dockerfile
tawdry vapor
That's ok
Unfortunately I don't really know how to debug this because it is hard to reproduce
crude gyro
hang on, so
echo "##vso[task.setvariable variable=BASE_CHANGED;isOutput=true]False" does this work on debian?
tawdry vapor
Yeah, it's just an echo
crude gyro
it's a weird way to set an environment variable.
tawdry vapor
It's an azure thing
crude gyro
aha
green oriole
I don't know a lot about shell script, but why one has a star and not the other one?
tawdry vapor
I found this command git show a4a8805587dc8e011fc2aa4276cc71631c58702a:docker/base.Dockerfile
Which would have been helpful to have in the script
it would show the contents of the dockerfile from the previous commit
In fact I could just use that with the gnu diff command I guess lol
tawdry vapor
No, because setting the env var means saying "there are no changes"
crude gyro
hmmmmm. I'm not sure I understand that. does it default to true then?
and then is only changed to False in the first conditional branch?
tawdry vapor
Correct. The variable is called BASE_CHANGED and by default it is true. Therefore, by default, the base image is always built.
crude gyro
okay, I see. then yeah I guess that command must be returning incorrectly indeed.
molten bough
that's not right
crude gyro
would be interesting to see what the diff it returns is
green oriole
Wait, is the command executed in the good directory? If git can't find it, it exit with 0
molten bough
if there are changes then it should say "set the env var"
tawdry vapor
if git diff --quiet "${prev_commit}" -- docker/base.Dockerfile; then
echo "No changes detected in docker/base.Dockerfile."
echo "##vso[task.setvariable variable=BASE_CHANGED;isOutput=true]False"
else
# Always rebuild the venv if the base changes.
exit 0
fi```since the return code is 1 and not 0
crude gyro
the return code doesn't matter in this case.
tawdry vapor
Maybe this is easier to digest
if return_code == 0:
BASE_CHANGED = False
elif return_code == 1:
BASE_CHANGED = True
...
if BASE_CHANGED is True:
build_base_image()
It does? The if is based on the return code
crude gyro
yeah, the git diff return code matters
molten bough
since the return code is 1 and not 0
molten bough
yeah, I'm not sure why Discord did that
I sent it right after the "if there are changes" message
crude gyro
@tawdry vapor maybe we could put some more debugging prints in here - like showing the full output of git diff "${prev_commit}" ?
and then run another build to see
might help us figure out what the fuck is going on
tawdry vapor
I could try. I will open a test PR
It isn't consistently reproducible. As I mentioned, most of the time it does work properly.
green oriole
And do a little pwd
crude gyro
one thing I don't fully understand, @tawdry vapor
tawdry vapor
yes?
crude gyro
so, say you make a commit now
if you diff that commit against the previous commit, there won't be any changes in the dockerfile
I mean, unless you change it.
tawdry vapor
Yes.
crude gyro
so will it ever work again if we miss one like this?
tawdry vapor
It is a test PR so I will change the dockerfile, like adding a newline
crude gyro
or did we like.. miss the bus on this working because it failed once?
yeah yeah you'll change the dockerfile this time, but what I'm getting at is
what happens when we merge a PR with many commits, only one early one of which changes the dockerfile
won't it just build the latest commit, then diff against the previous one before that, and never see the dockerfile diff?
tawdry vapor
It uses the azure API to fetch the commit hash of the most recent successful build
crude gyro
hmm.
tawdry vapor
basically it compares changes between the last build and the current one
hardy gorge
Does that include builds for commits/PRs?
hardy gorge
So, if it compares the latest with the commit build before it, it won't see a dockerfile change
I'm just walking in to this conversation and I'm probably totally wrong, though
green oriole
Well, only 93da is a changé to the dockerfile
tawdry vapor
I think you misunderstand how git diff works. You give it a commit to compare against and it looks at the entire state of the repo and how the files were then. It doesn't matter if that exact commit didnt change the dockerfiles.
crude gyro
yeah. I was about to type something to that effect
hardy gorge
right, I thought you said it compared the current build against the last succesfull build
tawdry vapor
Yeah it does
green oriole
The thing is that it output the compared commits, and if we run it locally we don't get the same output
hardy gorge
I'll scroll up to read more
green oriole
https://discordapp.com/channels/267624335836053506/635950537262759947/656230442420142090 if you wanna catch up
tawdry vapor
You did lead me to look something up though
hardy gorge
Because I just see two successful builds, if it compares the latest build against the last succesful one, there's no difference in dockerfile, but apparently you're talking about git diff with the entire repository
so I missed something
tawdry vapor
I looked into the PR, the build where I changed to debian
It did the same thing
it couldnt detect changes to dockerfile
green oriole
So it never built Debian images?
crude gyro
diffing against last successful build seems reasonable to me, as long as the conditional to detect the first change works. I mean, at some point you're going to merge something, either a commit or a group of commits, and the latest in that group of commits will always have a diff against the last successful build. however, as soon as that fails one time, you now have a last successful build with your dockerfile changes in it, so now it's fucked.
tawdry vapor
Yeah I get you
that means if we wanted to force a build and push
we'd need to change the dockerfile
crude gyro
but if it never fails that first time, this would work
then again - it shouldn't have failed this first time.
so.. if this isn't gonna work 100% of the time, this solution may not be feasible.
tawdry vapor
I agree
But I am not gonna give up yet
We haven't really made sense of why the diff is failing
crude gyro
yeah
so let's figure that out by adding that debug print
and then making some dockerfile changes
hardy gorge
How does it determine that last commit with a succesful build?
crude gyro
azure api
hardy gorge
Okay, I need to read up on the conversation, because I'm probably looking at the wrong point in history.
green oriole
The get_build method
hardy gorge
Which "we did not build" are we talking about? The last merge? Or Mark's commit that doesn't have a build associated with it?
tawdry vapor
The base and venv images were not built when PR was merged to master.
hardy gorge
Okay
tawdry vapor
Furthermore, I noticed that the git diff also reported no changes for the PR build after I changed the dockerfile to Debian.
hardy gorge
The last two commits in the git history of the repository have builds associated with them, if you diff just those two, it will show no changes for the base.Dockerfile
tawdry vapor
It looks for a build on the master branch, not on the PR branch
crude gyro
yes ves, but that's because this system is in an erronious state now.
this never should've been possible
hardy gorge
sebastiaan@N53SV:~/pydis/repositories/snekbox
[master=] -> $ git diff "707cd56" -- docker/base.Dockerfile
sebastiaan@N53SV:~/pydis/repositories/snekbox
okay
green oriole
The ci is comparing those two commits anyway
Retrieving the latest successful build using https://dev.azure.com/python-discord/96ac3876-1f84-4571-935f-c5bd3862b804/_apis/build/builds?queryOrder=finishTimeDescending&resultFilter=succeeded&$top=1&repositoryType=GitHub&repositoryId=python-discord/snekbox&api-version=5.0&branchName=refs/heads/master
Comparing HEAD (4d41ba2bbcde47d1304d25c53953e5de62c24d64) against a4a8805587dc8e011fc2aa4276cc71631c58702a.
No changes detected in docker/base.Dockerfile.
No changes detected in docker/venv.Dockerfile or the Pipfiles.```@hardy gorge The script prints out the commits it is comparing. You can run a diff manually and see that there should be a diff
It does not compare the second commit you have boxed in red
because that build was not ran on the master branch
I know the commits its comparing are correct, that isn't the problem
hardy gorge
right
crude gyro
basically ves, it relies on this working 100% of the time. the first time when the dockerfile was changed there would have been a diff, and that should've triggered the new build and dockerhub push. but it didn't, so now this system thinks our dockerfile is nice and debiany when it actually isn't.
and we did check the specific hashes that it said had no diff
and they do have a diff
so the check is fucked.
somehow.
hardy gorge
yes, okay. I'm just trying to figure out how this works.
crude gyro
yeah, I was doing the same thing
green oriole
That was fast
crude gyro
that's the output for the git diff?
crude gyro
oh
tawdry vapor
green oriole
Told you, not in the right directory (I have no idea actually haha)
tawdry vapor
this is what I added to the yml sh git show "${prev_commit}:docker/base.Dockerfile" git diff "${prev_commit}" -- docker/base.Dockerfile diff -q <(git show "${prev_commit}:docker/base.Dockerfile") docker/base.Dockerfile echo "$PWD"
oh god that syntax highlighting
green oriole
Well, you should move it higher up
crude gyro
well, there's a .dockerignore that ignores everything we haven't made an exception for
so the dockerfile wouldn't be there, would it?
tawdry vapor
dockerignore is only used by the docker cli when it builds images
so not relevant here
it just says "dont put this stuff into the docker image"
tawdry vapor
these commands are running in the azure agent not in the container
Working Directory Working directory in which you want to run the script. If you leave it empty it is folder where the script is located.
so cwd is in the scripts folder
god that's so fucking stupid
why doesnt git say anything if the file doesnt exist
crude gyro
so if we just set the cwd at the top of the script, could that be a solution for this whole thing?
tawdry vapor
yes
tawdry vapor
yeah we can change the cwd for the task that runs the script
crude gyro
sounds promising, nice find
hardy gorge
That's interesting. A Bash Task will use the SourcesDirectory as the default directory, but I was looking at the wrong task type.
Working Directory Specify the working directory in which you want to run the command. If you leave it empty, the working directory is $(Build.SourcesDirectory).
Which would have been what Mark expected
But, that's a bash task, not a script task
So, the confusion is not entirely odd; similar tasks have different default behavior
tawdry vapor
Well I tried to set the cwd but it didn't set it...
green oriole
Microsoft want you to do it the azure way haha
hardy gorge
You can try $(Build.SourcesDirectory) in the pipelines file
tawdry vapor
I did
The docs just suck and are innacurate
I had to look at the source code
which has better docs
hardy gorge
Oh
green oriole
Looks like it has worked?
crude gyro
looks like it's building the images,
hardy gorge
What's the difference between using a "Bash task to run a Bash script" and a "Shell Script task that runs a script using bash" in this context?
hardy gorge
nevermind a bit off topic, I'm trying to wrap my head around the azure docs
tawdry vapor
yeah let me clean up the pr 😄
crude gyro
yeah np
tawdry vapor
I dunno why though
the stuff did not print out
all of this
echo "$PWD"
ls -la
ls -la ..
git show "${prev_commit}:docker/base.Dockerfile"
git diff "${prev_commit}" -- docker/base.Dockerfile
diff -q <(git show "${prev_commit}:docker/base.Dockerfile") docker/base.Dockerfile```nowhere to be seen
crude gyro
yeah, I noticed that too
tawdry vapor
oh
exit 0
thats why
I'll need to change the docker file to force a rebuild I suppose
tawdry vapor
Thank you all for helping fix this
crude gyro
fucking CI problems
2 hours of troubleshooting and debugging and it culminates in something like disableAutoCwd: true
crude gyro
seems fair
green oriole
Azure not setting the cwd to the root of the repo is also kind of silly
tawdry vapor
Holy fuck does this take ages to build
I wanted to revisit this eventually anyway
I learned some tricks
That's for another day
green oriole
5-6 minutes last time
crude gyro
just takes a while when you actually have to build it
tawdry vapor
Yeah true
crude gyro
but most of the time, we don't.
tawdry vapor
Unfortunately all of the recent changes have been changes to the docker images
so we haven't see it be fast
green oriole
At least, that was the last project to switch to the new image right? It is all done and dusted now
tawdry vapor
I dunno
green oriole
Fair enough
Hmm, when the PR will be merged, the CI will rebuild the container again?
tawdry vapor
Yes
molten bough
well, my discrim doesn't start with 0, I guess
crude gyro
true
molten bough
Should be an easy fix anyway
hardy gorge
Yes, it's probably easy to remedy by adjusting the string representation
It shows up in more places
crude gyro
it's more than just the representation though. I don't have any roles
presumably because it can't look me up
molten bough
It should be using your ID, though
that definitely worked
yes, pycharm, open the repo I just cloned..
crude gyro
so there's something weird going on here
patent pivot
!sync users
stable mountainBOT
👌 User synchronization complete, created 64 and updated 64 users.
crude gyro
don't try to walrus yourself out of this one, @molten bough
crude gyro
hmmm. syncing users is taking a lot longer than it should.
could it be they were all out of sync somehow?
molten bough
I mean, user_model_updated is tied to the discord User model being updated
it's going to run that for each row that gets updated Django-side
but I can't imagine there being that many changes
patent pivot
yeah it's syncing them all now I'm watching logs
crude gyro
so maybe that's why my roles are gone
patent pivot
on the plus side everyone we have made stupidly efficient software this thing is going WHOOSH
crude gyro
and why you can't create an account
molten bough
WHOOSH? I like WHOOSH
crude gyro
okay well then the 0001 thing is probably just a string representation issue
but we clearly have some sync problem
stable mountainBOT
Server information
Created: 2 years, 11 months and 8 days ago
Voice region: us-central
Features: PARTNERED, PUBLIC, VANITY_URL, BANNER, NEWS, VIP_REGIONS, ANIMATED_ICON, DISCOVERABLE, INVITE_SPLASH
Counts
Members: 29,587
Roles: 48
Text: 96
Voice: 6
Channel categories: 13
Members
3084
1974
1123
23406
molten bough
that feel when literally none of your pycharm plugins are compatible
crude gyro
takes a minute to make 30000 API calls
molten bough
it's making a call for every user?
patent pivot
yeah
molten bough
yikes
patent pivot
crude gyro
normally, making a call for every user would be fine
green oriole
Okay guys, sorry to bust in the conversation, but external libs that run C code apparently still doesn't work : #bot-commands
crude gyro
but when you have to sync the entire userbase, not so much
molten bough
I guess ideally it's not a thing you need to do much
crude gyro
we should never have had to do it after the initial sync
but clearly we've messed something up that makes it wipe the users
patent pivot
if i'm understanding these logs right then we just passed 20k
long garnet
did we lose the user table?
molten bough
Running in the 90s
crude gyro
@long garnet looks like it, we're resyncing it though so it's no problem.
long garnet
what about the other tables
crude gyro
no indication we have any issues with those.
molten bough
Well, the wiki is fine
alright, well, the line needed to be changed is here,
utils/account.py
patent pivot
sync should nearly be done
molten bough
I guess it could just be in the f-string actually
{discriminator:0>4} if I'm not mistaken
crude gyro
you were mistaken then?
molten bough
Gotcha
molten bough
That sounds like progress
patent pivot
yep, i just synced the roles
crude gyro
same here
molten bough
I take it that's a yes to my other question as well then
molten bough
They'd be deleted with a group or role that was deleted
did you check whether roles needed syncing.. before syncing?
crude gyro
I don't know. I didn't check the mappings
I remember them disappearing once before
so that's probably what's wrong
and the users had to be resynced at least once before, too
green oriole
_Have you checked the CI? _
crude gyro
so there's some weird stuff going on with sync
molten bough
do we think the problem is in the sync, or in the database?
crude gyro
CI has nothing to do with this
green oriole
Yeah yeah, that was a joke
crude gyro
what would be happening in the database?
molten bough
is it clustered or just a single instance?
crude gyro
just a single instance
molten bough
hm, probably not that then
green oriole
AND IT FINALLY WORK GUYS, WOOOO
molten bough
role mappings are never actually updated or deleted aside from being touched in the admin, or an associated group/role being deleted
crude gyro
my theory is that it receives an empty users sync update and then it says "oh okay, so I guess I have to remove the 30K users that are here and all the roles and roles mappings"
molten bough
the users are one thing, we're using an api library for that, but..
Well, I guess that's possible
crude gyro
like.. maybe the bot messes up the sync heartbeat every now and then and the API interprets this as encouragement to nuke fucking everything
so I guess we should investigate that
patent pivot
in the DB
crude gyro
yeah so those need to be added then
green oriole
I didn't remembered having issue with empty user sync on my local site and bot
patent pivot
also
molten bough
yes, role mappings, joph
are they there?
nobody has answered this lol
they're in the staff section
patent pivot
molten bough
there's your problem
patent pivot
okay so yeah
molten bough
I'm looking at the roles stuff in the API
it looks like you'd have to actually send a HTTP DELETE to remove a role
patent pivot
I can manually do these right
molten bough
which is good, because it's explicit
and it's also not cascading
yeah, you'll need to do them
there's no automated setup for them
molten bough
Don't worry, they're super simple
molten bough
Not unless you want them doing administrative stuff on the wiki
iirc
I think it provides access to the django admin too
green oriole
And see mod logs I think
molten bough
that should be all you need to do
patent pivot
ah wait, i'm logged in as admin
molten bough
oh, yeah
crude gyro
you found the bug?
patent pivot
yeah, there were no role mappings in the database
could not correlate discord roles to django groups
crude gyro
that's not the bug.
molten bough
the question is why they weren't there
patent pivot
right yeah
crude gyro
let's write up an issue maybe
explaining that some unknown event can cause all the users to get desynced, and the role mappings to disappear
I think there's a chance we need to coerce @molten bough into doing that bug hunt, since he wrote those systems.
molten bough
I've been hunting already
patent pivot
this is also probably not great
molten bough
the problem is that I have written zero code that does anything but read from these role mappings
the only reason django should remove them is from the admin panel, or if a role/group is removed that is associated
crude gyro
well.. if all the roles disappear, would they be able to exist?
molten bough
If the roles are gone, so are the mappings
crude gyro
right
so then the roles are gone
no users, no roles
syncing restores both
so something is causing both to be removed. and I mean, sync does have the power to remove users
molten bough
Ah okay
patent pivot
okay, resources page updated on the upside
crude gyro
again, I'm pretty sure the problem is that the bot somehow sends a falsy request to this sync endpoint and the endpoint says "oh okay, better just get rid of everything then". I mean, if the bot reports that 5 users left, it should remove 5 users, and it does that by saying "well I got this list of 29200 users" which sorta gets diffed against the 29205 users in the database
so if it sent over 0 users, then I imagine it would just say okay I guess you lost 30K users today
and update the database accordingly
which means we need some sort of protection against that.
molten bough
if that happens then it is quite likely to be discord's fault
which means safeguards, yeah
crude gyro
for one, huge syncs of 1000+ users should only ever be possible via the command
that would probably solve this on its own
we should also make sure that we require enough specificity in the endpoint
molten bough
I really appreciate how volcyy documented the viewsets
crude gyro
like, I guess we're validating against some schema? can we do more validation to eliminate bad calls? calling the endpoint with just empty containers shouldn't have any effect, neither should requests that indicate that something went wrong on the bot side. maybe we can do some proper error logging when big diff requests come in, maybe with an everyone ping in #dev-core or something. maybe we can validate a little on the bot side, making sure we get a good dataset before even calling the endpoint?
stuff like that.
molten bough
I mean, yes, it's 2 lines of code and 114 lines of docs, but it has everything you could possibly need to know
okay well the viewset for the user objects is itself two lines
basically just setting a serialiser and queryset
the serialiser is itself only around 6 lines of code
DRF is doing a lot of work
clever wraith
@crude gyro Can you please tell me how did you found picture of emojis hosted on https://images-ext-2.discordapp.net/ ?
molten bough
Right click one in chat, copy link
clever wraith
in web browser right
molten bough
Or desktop
crude gyro
@molten bough https://github.com/python-discord/site/commit/c8207ab9af12e9b068ca3362788c0c164c48e64a does not seem to have done the trick
molten bough
Does it work if you relog?
crude gyro
and I'm not sure the sync for discrims is working. when I change my discriminator to #9999 and log in, I get this
even if I delete the account
it still shows this
note the top right being correct.
molten bough
Alright, I'll take a look
crude gyro
and it also turns out that the role table was emptied last friday
so that's probably why the role mappings disappeared
molten bough
Yup, that would explain that
crude gyro
so we have a few mysteries to solve here regarding this system:
- Why the hell did the roles table get emptied, and how can we prevent it from happening again?
- Why did it need to sync all 30K members when we used !sync users the other day?
- Why isn't my discrim changes being synced to the website?
- How can we get zero padded discrims to show in the profile?
I haven't the faintest idea what the answer to these questions is, but I'm certain that if we don't figure it out at some point, it'll keep being a source of frustration
molten bough
hm, I had a feeling something was missing from the signals when I did the PR last time, but I couldn't figure it out
but looking at it, the API User object update signal isn't modifying django user info
crude gyro
the best suggestion I have would be to put in some safeguards to prevent it from completely emptying the user and role tables if discord API has some issues or something like that
but that seems like addressing the symptoms rather than the cause.
so I'd love to hear some better suggestions
molten bough
Even if we figure out the cause, that's probably a good thing to have anyway
crude gyro
well, sure.
molten bough
because Discord API has been known to be spotty
crude gyro
but it's not enough for me to be completely comfortable with this system, on its own
I do think it should probably reject extreme operations, though
molten bough
API Discord users are also storing the discrim as an int
hardy gorge
Can you change your discrim for just a second again, lemon?
Or anyone with nitro, it doesn't have to be lemon
molten bough
I'm a little wary of changing mine considering it's plastered all over the 'net, someone swiping it would be quite bad indeed
hardy gorge
sure
crude gyro
nobody else in the universe is called gdude
molten bough
wanna bet
crude gyro
and nobody wants 2002 as their discrim anyway :D
hardy gorge
haha
crude gyro
yeah, who are called gdude?
hardy gorge
can you humor me for one second with a discrim change lemon?
molten bough
bruh
crude gyro
oops, it says "you're changing it too often, sorry"
crude gyro
rate limited!
molten bough
womp
crude gyro
@tranquil topaz help!
hardy gorge
!int e
eivl = await bot.api_client.get('bot/users/172395097705414656')
print("Current state of eivl in the database", eivl)
stable mountainBOT
In [6]: eivl = await bot.api_client.get('bot/users/172395097705414656')
...: print("Current state of eivl in the database", eivl)
...:
Current state of eivl in the database {'id': 172395097705414656, 'avatar_hash': 'e5180c631dbf5b51262845f6278287c5', 'name': 'eivl', 'discriminator': 1134, 'roles': [267628507062992896, 267629731250176001, 267624335836053506, 295488872404484098, 267630620367257601, 352427296948486144, 463658397560995840, 585529568383860737, 587606783669829632, 490216785547755530], 'in_guild': True}
or @woeful thorn
molten bough
well I've found one instance to fix over here
not sure what's up with that template though, looking into it
gusty sonnet
Hmm I can change my descrim, but do I count lol
green oriole
If you have an associated role on the site, yes
tranquil topaz
sorry @crude gyro what?
hardy gorge
!int e
user = await bot.api_client.get('bot/users/128376038605586432')
print("Current state of the user in the database", user)
stable mountainBOT
In [10]: user = await bot.api_client.get('bot/users/128376038605586432')
...: print("Current state of the user in the database", user)
...:
Current state of the user in the database {'id': 128376038605586432, 'avatar_hash': '2dcceae2b68781e9093282d9ddba56e4', 'name': 'Shirayuki', 'discriminator': 3007, 'roles': [463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 267629731250176001], 'in_guild': True}
@tranquil topaz I was looking for someone with Nitro that was willing to change their discriminator for a second
!int e
user = await bot.api_client.get('bot/users/128376038605586432')
print("Current state of the user in the database", user)
stable mountainBOT
In [11]: user = await bot.api_client.get('bot/users/128376038605586432')
...: print("Current state of the user in the database", user)
...:
Current state of the user in the database {'id': 128376038605586432, 'avatar_hash': '2dcceae2b68781e9093282d9ddba56e4', 'name': 'Shirayuki', 'discriminator': 3007, 'roles': [463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 267629731250176001], 'in_guild': True}
hmm
gusty sonnet
It's 7003 now
hardy gorge
!int e
user = await bot.api_client.get('bot/users/128376038605586432')
print("Current state of the user in the database", user)
stable mountainBOT
In [12]: user = await bot.api_client.get('bot/users/128376038605586432')
...: print("Current state of the user in the database", user)
...:
Current state of the user in the database {'id': 128376038605586432, 'avatar_hash': '2dcceae2b68781e9093282d9ddba56e4', 'name': 'Shirayuki', 'discriminator': 3007, 'roles': [463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 267629731250176001], 'in_guild': True}
!sync users
stable mountainBOT
👌 User synchronization complete, created 0 and updated 0 users.
hardy gorge
!int e
user = await bot.api_client.get('bot/users/128376038605586432')
print("Current state of the user in the database", user)
stable mountainBOT
In [13]: user = await bot.api_client.get('bot/users/128376038605586432')
...: print("Current state of the user in the database", user)
...:
Current state of the user in the database {'id': 128376038605586432, 'avatar_hash': '2dcceae2b68781e9093282d9ddba56e4', 'name': 'Shirayuki', 'discriminator': 3007, 'roles': [463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 267629731250176001], 'in_guild': True}
rip bot
hardy gorge
okay...
Well, even a manual sync does not sync the discrim
!int e
user = await bot.api_client.get('bot/users/336843820513755157')
print("Current state of the user in the database", user)
stable mountainBOT
In [14]: user = await bot.api_client.get('bot/users/336843820513755157')
...: print("Current state of the user in the database", user)
...:
Current state of the user in the database {'id': 336843820513755157, 'avatar_hash': '27043118745b175fe1ddfbe7002c7069', 'name': 'Ves Zappa', 'discriminator': 3787, 'roles': [267627879762755584, 267628507062992896, 267624335836053506, 267629731250176001, 267630620367257601, 277914926603829249, 295488872404484098, 352427296948486144, 463658397560995840, 587606783669829632, 490216785547755530, 518565788744024082], 'in_guild': True}
I think it does not change anything
Does it change roles
And/or avatar hash as well as the in_guild
Will need a clone for that
hardy gorge
in_guild gets changed
!int e
user = await bot.api_client.get('bot/users/128376038605586432')
print("Current state of the user in the database", user)
stable mountainBOT
In [15]: user = await bot.api_client.get('bot/users/128376038605586432')
...: print("Current state of the user in the database", user)
...:
Current state of the user in the database {'id': 128376038605586432, 'avatar_hash': '2dcceae2b68781e9093282d9ddba56e4', 'name': 'Shirayuki', 'discriminator': 3007, 'roles': [463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 277914926603829249, 267629731250176001], 'in_guild': True}
mute yourself and see if your roles sync?
gusty sonnet
Ok
hardy gorge
!int e
user = await bot.api_client.get('bot/users/128376038605586432')
print("Current state of the user in the database", user, 277914926603829249 in user["roles"])
stable mountainBOT
In [16]: user = await bot.api_client.get('bot/users/128376038605586432')
...: print("Current state of the user in the database", user, 277914926603829249 in user["roles"])
...:
Current state of the user in the database {'id': 128376038605586432, 'avatar_hash': '2dcceae2b68781e9093282d9ddba56e4', 'name': 'Shirayuki', 'discriminator': 3007, 'roles': [463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 277914926603829249, 267629731250176001], 'in_guild': True} True
there.. its changed to something else
gusty sonnet
!mute 128376038605586432 7d being cute potato
hardy gorge
on Shiru
stable mountainBOT
:incoming_envelope: :ok_hand: applied mute to @gusty sonnet until 2019-12-24 14:30 (6 days and 23 hours).
hardy gorge
I just manually added the role
gusty sonnet
That works too
hardy gorge
so, the role was synced
molten bough
okay well that explains a lot
hardy gorge
[463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 277914926603829249, 267629731250176001]
[463658397560995840, 267624335836053506, 267630620367257601, 352427296948486144, 587606783669829632, 518565788744024082, 267629731250176001]
molten bough
and it also explains the behaviour lemon pinged me about
molten bough
the notification at the top right had the correct discrim because it came from oauth
hardy gorge
in_guild also syncs, I checked that on users who left the guild recently
@Cog.listener()
async def on_member_update(self, before: Member, after: Member) -> None:
"""Updates the user information if any of relevant attributes have changed."""
if (
before.name != after.name
or before.avatar != after.avatar
or before.discriminator != after.discriminator
or before.roles != after.roles
):
This is the relevant part that takes care of syncing users on update (not leave)
molten bough
but the settings modal uses user.username which would be relying on the database state
which.. yeah
there is a one-line fix you guys can push if you like
which means that the discrim padding will also be applied on API user update
apps/home/signals.py line 266
hardy gorge
I'm going to play with the syncer for a while, because something is up
molten bough
I did a project-wide search and there are no other non-test non-migration references to discrims
gusty sonnet
I'm dealing with firebase and it's hella stupid lol
molten bough
:o
hardy gorge
a change to the user details does not trigger a on_member_update, but an on_user_update event
gusty sonnet
fantastic
hardy gorge
So, your discrim/username changes will only be synced once a change to your member state triggers the check
Okay
I'll fix it
I'll make an issue first of course
gusty sonnet
This is called when one or more of the following things change:
status
activity
nickname
roles```I think these events are more or less in line with Discord API events
hardy gorge
I now know why some people got updated while others not
gusty sonnet
discord.on_user_update(before, after)¶
Called when a User updates their profile.
This is called when one or more of the following things change:
avatar
username
discriminator```If you had a status/activity change after your username/discrim change, the syncer would trigger
gusty sonnet
Amazing
So you need to do it on both?
We'll prob create a new function and plug it in both events, yes?
hardy gorge
Yes, one for the roles, one for the others
hardy gorge
Something like that. We can have a check for status...roles in on_member_update and avatar..discrim in on_user_update and then call the sync util
Now, the most difficult question of all: Which label do I need to use for this issue?
backend? API? utility?
woeful thorn
enhancement
hardy gorge
Hm
The on_user_update will receive a User instance, not a Member instance, so we need to do a bit of preparatory work to sync the user as we do now, since User does not have a roles attribute
another option would be to just update the attributes relevant to that event, instead of fetching a Member for that User
The latter seems far easier and the endpoint is okay with it
crude gyro
yeah, you still get the ID, right?
should be easy to just update whatever is relevant.
hardy gorge
yes
The syncers now always replace the entire user in the database with PUT, I'll turn that into a PATCH
crude gyro
do you wanna put in some safeguards against bad discord api responses annihilating our entire user and role tables, too?
something like a threshold for number of changes allowed with an autosync update?
maybe a webhook with a ping for these, too, so they're not invisible
ignoring requests to remove all roles or all users seems like a good place to start, I'm thinking.
hardy gorge
We could do that, although I have nearly fixed this thing, so I can also make two separate PRs, since this latter thing may require a bit more thinking about a proper design
molten bough
just to make sure it isn't missed, this one-line fix will be needed on the site as well https://canary.discordapp.com/channels/267624335836053506/635950537262759947/656504158601609247
(for discrim formatting)
hardy gorge
Does it influence any of the unit tests we do?
I need a sanity check, I can't get this to fire and I have no idea why:
@Cog.listener()
async def on_user_update(self, before: User, after: User) -> None:
"""Updates the user information if there was a relevant change in the user account information."""
log.debug(f"User change detected: {after}")
I'm probably missing something trivial.
Its sibling works like expected:
@Cog.listener()
async def on_member_update(self, before: Member, after: Member) -> None:
"""Updates the user information if there was a change in the roles of the member."""
log.debug(f"Member change detected: {after}")
I don't think it does, no
hardy gorge
Okay
molten bough
and those look.. basically exactly the same to me
hardy gorge
Yet, it fires for none of the changes listed in the docs (username, discriminator, avatar)
I'm going slightly crazy
I know it's registered:
In [4]: syncer = bot.get_cog("Sync")
...: print(syncer.get_listeners())
...:
[('on_guild_role_create', <bound method Sync.on_guild_role_create of <bot.cogs.sync.cog.Sync object at 0x7f7ed267cd50>>), ('on_guild_role_delete', <bound method Sync.on_guild_role_delete of <bot.cogs.sync.cog.Sync object at 0x7f7ed267cd50>>), ('on_guild_role_update', <bound method Sync.on_guild_role_update of <bot.cogs.sync.cog.Sync object at 0x7f7ed267cd50>>), ('on_member_join', <bound method Sync.on_member_join of <bot.cogs.sync.cog.Sync object at 0x7f7ed267cd50>>), ('on_member_remove', <bound method Sync.on_member_remove of <bot.cogs.sync.cog.Sync object at 0x7f7ed267cd50>>), ('on_member_update', <bound method Sync.on_member_update of <bot.cogs.sync.cog.Sync object at 0x7f7ed267cd50>>), ('on_user_update', <bound method Sync.on_user_update of <bot.cogs.sync.cog.Sync object at 0x7f7ed267cd50>>)]
caching issue? maybe use raw
hardy gorge
there's no raw for this
crude gyro
well, fuck.
hardy gorge
Anyway, I've confirmed that on_member_update definitely does not fire for those changes
But, neither does on_user_update from the looks of it
I've updated both my username and avater (can't do discriminator) but no ball
hardy gorge
No, only for username, avatar, discriminator
according to the docs
I cant' find an issue for it either, so maybe it's still something I'm doing
hardy gorge
Checked that, yeah
It would also prevent the on_member_update from firing, but that one definitely fires
molten bough
is guild_subscriptions set to False?
hardy gorge
No, it's True
molten bough
hmm
I can literally only find two references to on_user_update and they're a comment and a docstring
ah, then again, on_member_update only has one reference
hardy gorge
I was wrong
molten bough
just found the dispatch code, and it.. hm
crude gyro
how could you be wrong when it felt so right
hardy gorge
We don't have a guild_subscriptions yet, since that's 1.3.0
molten bough
Oh right
crude gyro
aha
hardy gorge
nevertheless, all the other events that are listed there fire
and on_user_update is definitely also in 1.2.5
Just not the subscriptions bool
I'll try a raw event listener not part of a cog
That doesn't do anything
It should be a valid event, taking https://discordapp.com/developers/docs/topics/gateway#user-update into account
crude gyro
yeah
I don't know why it's happening. I wonder if it would happen on 1.30
maybe you should bump the version and test
hardy gorge
I've changed the logging level of Discord to DEBUG first because it does log the events it's supposed to dispatch
let me try that
I'll update to the master branch after
Dec 17 17:14:01 Bot: | discord.client | DEBUG | Dispatching event user_update
hmm
ah
now it works
but why
I did not change anything but the logging level of discord.py
hardy gorge
Okay, it fires consistently now. I'm not sure what changed, but I'll take it
It's probably something
molten bough
Good ol' heisenbug
hardy gorge
Now, what's really interesting is that changing my username triggers:
Dec 17 17:49:27 Bot: | bot.cogs.sync.cog | DEBUG | User change detected: Ves Zappa#3787
Dec 17 17:49:27 Bot: | bot.cogs.sync.cog | DEBUG | Member change detected: Ves Zappa#3787
Dec 17 17:49:27 Bot: | bot.cogs.sync.cog | DEBUG | Member change detected: Ves Zappa#3787
So, both the on_member_update and on_user_update now trigger
crude gyro
what if you set a nickname
hardy gorge
did that
crude gyro
would it still trigger on username change?
hardy gorge
Yes
crude gyro
should I change my discrim?
hardy gorge
One second
Okay, the on_member_update does fire, but it does not contain any of the changes we're looking for between after and before
The on_user_update sees this:
Name: Ves Zappa -> Ves Zappa
Disc: 3787 -> 3787
Avat: 22e7db4594219321e665cbd810434c40 -> 3a305b2d34f677dd1c050bc98dd8e390
while on_member_update sees this for the same change event:
Name: Ves Zappa -> Ves Zappa
Disc: 3787 -> 3787
Avat: 3a305b2d34f677dd1c050bc98dd8e390 -> 3a305b2d34f677dd1c050bc98dd8e390
So, the member update has the avatar hash of the new avatar for both before and after, but it still fires. I'll need a DeepDiff to know why, but it's not really relevant
molten bough
Are those both the same event?
crude gyro
no that was obviously a typo
molten bough
Ah okay
hardy gorge
A DeepDiff sees no changes
OKay
so, an on_member_update stays useless for the changes we're looking for
I think I figured it out; it's a bit redundant but here it goes: When a user updates their account information, it will fire both the user_update event as well as the member_update event. I get two of the latter, because my test bot shares two guilds with my test user. The user_update event is processed before the member_update event nearly every time, which means that by the time the event listener for on_member_update runs, both the before and after Member objects already contain the changes (those Member objects will be fetched from the Member cache which is already updated by the user_update). So, it runs with a before and after that are equal. Pretty useless. The on_user_update runs with the "raw" User objects and those do reflect the changes.
So, while the discord.py documentation lies (on_member_update does fire for public account setting changes), the event listener will not get relevant information to go by.
Anyway, it works now
molten bough
\o/
mellow hare
Remind me: other than the bot token, what does my bot's .env file need?
green oriole
You use docker or not?
mellow hare
Yeah, docker
I run the site in docker from the site repo and then the bot repo
Because I like to be complicated
green oriole
You probably also need COMPOSE_PROJECT_NAME=site
mellow hare
I don't remember it needing that... I feel like it need the bot token thing that the site provides
hardy gorge
You'll need some dummy reddit oauth values as well, since the bot will crash if you don't have those
mellow hare
Ah, I've missed quite a few things then
green oriole
It doesn't for me thought..
hardy gorge
that's very recent
It caused the last major bot downtime we had last week
Traceback (most recent call last):
File "/home/sebastiaan/.local/share/virtualenvs/djangobot-Py5o4YeS/lib/python3.7/site-packages/discord/ext/commands/bot.py", line 580, in _load_from_module_spec
setup(self)
File "/home/sebastiaan/pydis/repositories/djangobot/bot/cogs/reddit.py", line 293, in setup
bot.add_cog(Reddit(bot))
File "/home/sebastiaan/pydis/repositories/djangobot/bot/cogs/reddit.py", line 38, in __init__
self.client_auth = BasicAuth(RedditConfig.client_id, RedditConfig.secret)
File "/home/sebastiaan/.local/share/virtualenvs/djangobot-Py5o4YeS/lib/python3.7/site-packages/aiohttp/helpers.py", line 123, in __new__
raise ValueError('None is not allowed as login value')
ValueError: None is not allowed as login value
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "/usr/local/lib/python3.7/runpy.py", line 193, in _run_module_as_main
"__main__", mod_spec)
File "/usr/local/lib/python3.7/runpy.py", line 85, in _run_code
exec(code, run_globals)
File "/home/sebastiaan/pydis/repositories/djangobot/bot/__main__.py", line 45, in <module>
bot.load_extension("bot.cogs.reddit")
File "/home/sebastiaan/.local/share/virtualenvs/djangobot-Py5o4YeS/lib/python3.7/site-packages/discord/ext/commands/bot.py", line 625, in load_extension
self._load_from_module_spec(lib, name)
File "/home/sebastiaan/.local/share/virtualenvs/djangobot-Py5o4YeS/lib/python3.7/site-packages/discord/ext/commands/bot.py", line 584, in _load_from_module_spec
raise errors.ExtensionFailed(key, e) from e
discord.ext.commands.errors.ExtensionFailed: Extension 'bot.cogs.reddit' raised an error: ValueError: None is not allowed as login value
Ah, that would do it. My bot dev env has been out of commission since last week
Only just got it back up
I just need to make sure I have the right .env stuff and I should be good to do stuff again
hardy gorge
That's the traceback you'll see if you're missing REDDIT_CLIENT_ID and/or REDDIT_SECRET
They can be dummy values; then it simply won't be able to login
green oriole
For me the bot start, but the cog fail to load
hardy gorge
but not having them will crash the bot
Well, it crashes both on my system and it crashed in production because of that
green oriole
Oh wait, yeah it just went down
mellow hare
2 "blah"s it is
green oriole
Can we just have the bot itself using a dummy value, to avoid having adding it ourselves?
hardy gorge
I think it would be better to handle the exception and unload the cog when it happens to cover all bases
mellow hare
Throw it on the pile
hardy gorge
Instead of it causing the bot to crash completely
tawdry vapor
That should not be needed
tawdry vapor
The compose file sets one by default
mellow hare
I think it does for the bot compose but not for site
hardy gorge
?
mellow hare
Unless that got changed
tawdry vapor
That may be true
mellow hare
I run the site repo's docker, not the bot's
hardy gorge
Why does the site compose need one?
The site creates the token, right?
I do, too
Why does the site compose need one?
mellow hare
I'm saying the bot needs it in the .env to give it to the site
tawdry vapor
Sorry thought you were running bot compose
hardy gorge
Discord, why do you do this to me? I'm sorry if my messages showed up out of order
mellow hare
No no, I didn't explain it correctly
hardy gorge
For me, it shows duplicate messages now
mellow hare
I've got my site .env still set up from last time
Doesn't show that for me, Ves
You're good on my end
Unless we changed anything with the site's .env
hardy gorge
If you're using both docker for bot and for site, I don't think you should have to set those manually
mellow hare
Right
Exactly
Well we'll know in a second if I have the right .env's set if it starts pinging everyone
past falcon
WIP. Any thoughts on what to do about the header? Can't get the alignment quite right with those emojis. Also, is this just plain too wide for mobile? Thought it'd be neat to show times for both stars at once, but maybe it's a bad idea
woeful thorn
Emojis aren't monospaced so you're not going to get it perfect
tawdry vapor
It probably does look bad on mobile though, portrait orientation at least
green oriole
Well, everything related to the aoc is broken on mobile, so do we care haha
Looks pretty good anyway!
tawdry vapor
If you're sympathetic to mobile users perhaps add a command that only shows a single star rather than all for the day
past falcon
Good idea
jade tiger
@past falcon how does it look on mobile? Tables are a shocker to try and make look nice with PC/mobile
Sorry I can't read
Lol
Looks nice otherwise tho
What happens if you have users with emojis in name that are in the 1 star column, tho?
Pretty sure that'll throw it off for that row
hardy gorge
Okay, I've added a large issue here: https://github.com/python-discord/bot/issues/704
I decided to write the entire background down in case we run into similar issues in the future
We still need to decide on the proper implementation (or even if we want to go with these solutions)
crude gyro
excellent work
long garnet
mmm I want some specific clarification on the way proxy_user is supposed to work, it seemed it was used as a converter but it never got to be executed because there was never a case where a user couldn't be found among the synced ones (User, Member) and was in the DB at the same time
def proxy_user(user_id: str) -> discord.Object:
"""
Create a proxy user object from the given id.
Used when a Member or User object cannot be resolved.
"""
log.trace(f"Attempting to create a proxy user for the user id {user_id}.")
try:
user_id = int(user_id)
except ValueError:
raise commands.BadArgument
user = discord.Object(user_id)
user.mention = user.id
user.avatar_url_as = lambda static_format: None
return user```I don't understand why the fields there are those, `.mention` and `.avatar_url_as`?the issue is that if the user never joined the server and we rely on this, then proxy_user (as a last resource, even after fetching it from the Discord API) needs to fill the data to post a new user:python payload = { 'avatar_hash': user.avatar, 'discriminator': int(user.discriminator), 'id': user.id, 'in_guild': False, 'name': user.name, 'roles': [] }
tawdry vapor
Hi did you see my comment on your PR?
long garnet
I did, I was camping for you (?)
tawdry vapor
Also, those attributes are there to mock a User object. Some code doesn't check the type of the object is dealing with and just expects certain attributes to be available.
tawdry vapor
Presumably yes but I cannot cite a specific instance that I have observed it to be used by live code
long garnet
nothing, I'm just doing my homework to understand it
tawdry vapor
Ah ok
long garnet
does it exist to work only if a user is in the DB, and the infraction application couldn't get a proper User/Member?
tawdry vapor
Well I suppose. That statement is more correct if we ignore the DB part of it. I believe that when the proxy was created, the fact that a user needs to be in the database was likely not even considered.
But in practice yes, it will only work if the user is in the DB since it doesnt post to the db itself
That's the reason I created the issue
long garnet
ah, I see
well it worries me a bit then
if we allow commands to use proxy_user as a converter, then it has to post that fake user to the DB
which could be synced later, sure
but it also means something like !ban <random_numbers> would post a new user
tawdry vapor
I mentioned that if the fetch fails due to a user not being found then the proxy should not be used
tawdry vapor
So it's covering edge cases where the Discord API is being weird
long garnet
can the chain of converters be broken in some way? lmao
tawdry vapor
But because we don't really know how the API behaves (it could fail for some other reason before it even realises the user doesn't exist), it could indeed still end up creating a fake user in the database.
I don't really see that as an issue though
long garnet
I guess the Converter that fetches could return something to indicate it shouldn't be posted?
tawdry vapor
I think you should just make your new converter behave the like the proxy because that is simpler than trying to break the union chain or whatever
To clarify, I mean if it catches an exception from the fetch it will instead return a mock user object
tawdry vapor
and if the exception is a 404 or whatever one means not found then just raise an error
long garnet
thanks for the clarifications
tawdry vapor
np
tawdry vapor
you're doing well
long garnet
and DBs tbqh and docker
long garnet
@tawdry vapor thanks for the help, you're quite active in the repos