-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(backend): GitHub token should be a SecretStr #6494
base: main
Are you sure you want to change the base?
Conversation
filtered_settings_data['llm_api_key'] = settings_with_token_data.llm_api_key | ||
filtered_settings_data['github_token'] = SecretStr( | ||
settings_with_token_data.github_token | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was llm_api_key
converted too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I've had to do this here since this was the first instance where we receive and use github_token
but need it to be a SecretStr
. I think llm_api_key
is secret-ed via the serializer but I'm not certain. If it is, then maybe I can do the same with the github_token
but I encountered a bunch of issues when I tried.
I'll wait for @tofarr to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
llm_api_key was previously converted to a secret str - but github token was added as a regular str after this was done and needed the same conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I don't think the secret key conversion is strictly needed for either here - since the constructor for Settings will do the conversion if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from openhands.server.settings import Settings
Settings(llm_api_key='foobar')
>>> Settings(language=None, agent=None, max_iterations=None, security_analyzer=None, confirmation_mode=None, llm_model=None, llm_api_key=SecretStr('**********'), llm_base_url=None, remote_runtime_resource_factor=None, github_token=None, enable_default_condenser=False)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filtered_settings_data['github_token'] = settings_with_token_data.github_token
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make a unit test with both, to see it works as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Just a couple little questions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - the only nit I have is that we don't need that explicit conversion
End-user friendly description of the problem this fixes or functionality that this introduces
Instead of simply being type
str
, it should beSecretStr
like thellm_api_key
Give a summary of what the PR does, explaining any non-trivial design decisions
SecretStr
GETSettingsModel
andPOSTSettingsModel
Link of any specific issues this addresses
To run this PR locally, use the following command: