[release auto] use env var for twine password #59068
Conversation
so that failed twine upload will not print the token out in logs Signed-off-by: Lonnie Liu <lonnie@anyscale.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves security by using an environment variable for the twine password, preventing it from being logged in case of an error. The changes are correct and effectively address the issue. I've added a couple of suggestions to improve the flexibility and maintainability of the _call_subprocess helper function by making the new add_env parameter optional.
| import subprocess | ||
| import sys | ||
| from typing import List | ||
| from typing import Dict, List |
There was a problem hiding this comment.
To support making add_env an optional argument in _call_subprocess as suggested in another comment, Optional should be imported from typing.
| from typing import Dict, List | |
| from typing import Dict, List, Optional |
|
|
||
| def _call_subprocess(command: List[str]): | ||
| subprocess.run(command, check=True) | ||
| def _call_subprocess(command: List[str], add_env: Dict[str, str]): |
There was a problem hiding this comment.
The signature of _call_subprocess has been changed to require the add_env parameter. While this works for the current use case, it makes the function less reusable and breaks its previous contract. A more robust approach would be to make add_env an optional parameter. This would maintain backward compatibility and make the helper function more flexible for future use. You would also need to adjust the function body to handle the case where add_env is None.
| def _call_subprocess(command: List[str], add_env: Dict[str, str]): | |
| def _call_subprocess(command: List[str], add_env: Optional[Dict[str, str]] = None): |
so that failed twine upload subprocess call will not print the token out in logs as part of the exception.