Add function to process the passcmd for API key retrieval. #866
Conversation
Gopinath-Mahendiran
force-pushed
the
modifing_client
branch
3 times, most recently
from
8454aee to
0e6f7af
Compare
There was a problem hiding this comment.
@Gopinath-Mahendiran Thanks for giving the porting of the other PR across to this repo a try! 👍
I've left some comments inline, but the primary issue from my point of view is that we need to retain backwards compatibility, rather than just looking for passcmd instead of key.
The challenge with this repo (and module) is that there are few automated tests to start with, so it's important to check your changes carefully, or potentially add more tests first.
Much like the ZT PR has a documentation update, we'll want a similar update here too, for the new functionality.
zulip/zulip/__init__.py
Outdated
| api_key = config.get("api", "key") | ||
| passcmd = config.get("api", "passcmd") | ||
| api_key = self.get_api_key(passcmd) |
There was a problem hiding this comment.
This does not provide passcmd as a fallback to key, in the configuration file, but only appears to replace it? Running this PR/branch with current zulip-term main causes a crash, since my default zuliprc does not contain a passcmd, but only a key. You can reproduce this by entering the zulip-term virtual environment and installing the /zulip/ package folder in editable (-e) mode.
Also see my comment here: zulip/zulip-terminal#1581 (comment)
(while that comment applied to the documentation, this applies just as much to the code)
zulip/zulip/__init__.py
Outdated
| def get_api_key(self, passcmd: str) -> Optional[str]: | ||
| # run the passcmd command and get the API key | ||
| result = subprocess.run(passcmd.split(), capture_output=True, check=False) | ||
| if result.returncode == 0: | ||
| return result.stdout.decode().strip() | ||
| else: | ||
| raise RuntimeError("Error: Unable to retrieve API key.") |
There was a problem hiding this comment.
I'd suggest we don't need this to be a method that looks developer-facing, and it could likely be placed inline directly in the code above where it would be otherwise called.
That said:
- You're currently returning
Optional[str], but I don't see where that can happen; it's unclear to me why the type-checker isn't complaining about that - RuntimeError is a very broad exception; if we're to use an exception here, or differently if this is inlined, I'd suggest it should be more specific
- The returncode appears not to be portable across platforms;
runcan provide another solution, I believe? - While we rely on a user to know what they are doing when using passcmd, I would suggest that the simpler commands we accept, and/or the more checking of the command we can do, the better.
There was a problem hiding this comment.
now look at the changes and give the feedback
|
@Gopinath-Mahendiran It would help to know how you tested this - not everything is applicable here, but you didn't fill out the self-review checklist, for example. |
|
@neiljp I made the changes directly in the package on my local machine by replicating the modifications from this PR. |
Gopinath-Mahendiran
force-pushed
the
modifing_client
branch
from
0e6f7af to
a1acb67
Compare
Gopinath-Mahendiran
force-pushed
the
modifing_client
branch
3 times, most recently
from
52e7eb8 to
f448f7a
Compare
Gopinath-Mahendiran
force-pushed
the
modifing_client
branch
from
f448f7a to
c388e71
Compare
This commit is a continuation of PR #1581 in the zulip-terminal repository.
It adds functionality to retrieve the API key by executing a passcmd specified in the config file, rather than directly reading the key from the config. Once the passcmd is executed and the API key is obtained, the usual authentication flow proceeds as before.
How did you test this PR?
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: