search

zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
350 stars 356 forks source link

zulip-botserver: Allow passing config via environment variables #633

Closed LoopThrough-i-j closed 3 years ago

LoopThrough-i-j commented 3 years ago

Fixes #485

LoopThrough-i-j commented 3 years ago

@timabbott, or @showell can you please take a look at this PR

showell commented 3 years ago

@LoopThrough-i-j Thanks for working on this! I appreciate that you took the time to write tests here.

I am not sure having the parallel comma-separated lists of values here is ideal. I wonder if there's a better way to represent hierarchical data in environment variables.

I think we need to wait and see what Tim says here. (He and his wife just welcomed a baby into the world, so he's gonna be pretty busy for the next couple weeks, so apologies in advance if this doesn't get reviewed for a while.)

LoopThrough-i-j commented 3 years ago

Not a problem, thanks for the reply.

timabbott commented 3 years ago

I agree with the concern about comma-separated values in environment variables.

I'm not convinced that it's worth supporting multiple bots with this environment variables model; if we did, probably it'd be better to just have each bot under a prefix (E.g. GIPHY_ZULIP_BOT_EMAIL or something like that) and pass which prefixes to look for on the command line.

@pirate FYI for thoughts on that question.

pirate commented 3 years ago

I don't mind CSV strings personally, but a reasonable alternative is requiring proper json strings and just using json.loads (like ZULIP_CUSTOM_SETTINGS).

LoopThrough-i-j commented 3 years ago

Thanks for the reply, I agree with JSON strings as well, will update the pr soon.

LoopThrough-i-j commented 3 years ago

@pirate what do you think. Does it look fine🙂?

pirate commented 3 years ago

looks awesome, thanks for this work @LoopThrough-i-j!

LoopThrough-i-j commented 3 years ago

looks awesome, thanks for this work @LoopThrough-i-j!

No problem looking forward to work more.

LoopThrough-i-j commented 3 years ago

@timabbott can you please merge this if it is done.

showell commented 3 years ago

I haven't tested this or read it thoroughly, but the overall approach seems reasonable now. Is there a place in the readme that we should be updating too?

LoopThrough-i-j commented 3 years ago

I haven't tested this or read it thoroughly, but the overall approach seems reasonable now. Is there a place in the readme that we should be updating too?

The readme does not talk about the new feature, of passing config via environment variables. Then the docs will need an update as well along with the readme.

LoopThrough-i-j commented 3 years ago

@alexmv Thanks a lot for the suggestions. Can you please take a look if everything looks good?

The checks fail because in python 3.5 a random bot is chosen to replace the name instead of the 1st one. For some reason, Ordered Dict is not working.

Can you help to improve or shall I remove the test for checking the 1st python bot is replaced while using the environment variables?

Thanks a lot in advance.

LoopThrough-i-j commented 3 years ago

This looks pretty good! Just a few minor cleanups.

Did you have any luck tracking down the Python 3.5 problem?

I tried a lot of things out even printing down types and trying to find out leakages, but nothing worked(Stackoverflow couldn't help as well). Later, I decided to drop the test for python3.5 and the test happens only for python3.6 or higher.

LoopThrough-i-j commented 3 years ago

@alexmv what do you think

ryanreh99 commented 3 years ago

The checks fail because in python 3.5 a random bot is chosen to replace the name instead of the 1st one. For some reason, Ordered Dict is not working.

I didn't run this locally but I think this maybe the reason:- In the docs for json.load it is mentioned: All optional parameters are now keyword-only. And in the OrderedDict docs, it is mentioned:

Changed in version 3.6: With the acceptance of PEP 468, order is retained for keyword arguments passed to the OrderedDict constructor and its update() method.
LoopThrough-i-j commented 3 years ago

@timabbott can you please take a look at this PR.

Thanks a lot @ryanreh99 for pointing out the issue.

alexmv commented 3 years ago

Using object_pairs_hook=list to preserve the order is an interesting solution, that nicely steps around the OrderedDict problem. Since it's not necessarily completely intuitively obvious, I think it probably deserves extending the comment above it about "the first bot" that we re-parse to extract key order, in order to support Python 3.6 and before, since 3.7 and later would preserve key ordering information.

Otherwise, this is looks in great shape -- sorry for the long delay in the review!

LoopThrough-i-j commented 3 years ago

Using object_pairs_hook=list to preserve the order is an interesting solution, that nicely steps around the OrderedDict problem. Since it's not necessarily completely intuitively obvious, I think it probably deserves extending the comment above it about "the first bot" that we re-parse to extract key order, in order to support Python 3.6 and before, since 3.7 and later would preserve key ordering information.

sure will fix the issue and will be working on my next PR! btw the tests fail for now, will it be fine I take the responsibility to migrate it to Github actions.

alexmv commented 3 years ago

btw the tests fail for now, will it be fine

If the tests are still failing, that means the workaround of using list isn't working -- so no, that's not fine.

I installed python 3.5 myself to investigate, by putting some print statements into the test and running it several times. What jumped out is that the order of the list was still changing! So I added a print statement for the environment variable itself that was being fed into the test. Which showed that the problem isn't actually that the OrderedDict trick doesn't work during loading on 3.5 -- it's that we're using json.dumps to construct the JSON we put in the environment, which is unordered on Python 3.5, so we're feeding the program unreliable input!

The fix is to swap back to the much less hacky OrderedDict, and also to use that in the tests to make sure we pass in a consistent value for the environment variable. With that in place, tests pass consistently on 3.5, and later. I've pushed a commit that does this, as well as add some documentation.

will it be fine

Tests should always pass. In general, it's not a good idea to break functionality, even if you're planning on coming back and to fix it later -- particularly not when just adding new features, like this. If tests are failing when someone arrives at a project, folks will assume that any failure is probably not due to their commit, and thus failures (including real important ones!) will accumulate over time. Keeping confidence that tests don't lie is really important.

I take the responsibility to migrate it to Github actions.

I'd love to take you up on migrating to Github Actions, however! But let's do that in a separate PR.

LoopThrough-i-j commented 3 years ago

Thanks a lot for your help and detailed comment and for mentoring me throughout the PR. I would love to work on the migration of tests to GitHub actions. Thank you once more for helping me to become a better developer.