-
Notifications
You must be signed in to change notification settings - Fork 25
fix: skip starlette check when server version is hidden #696
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
Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
CI failures are probably #694 |
Thank you, I was trying to figure out how my change could possibly have caused this! |
rsconnect/main.py
Outdated
# 2024.01.1 or later, this can be removed. Requires access to the | ||
# Connect server version, which may be hidden. | ||
connect_version_string = ce.client.server_settings()["version"] | ||
if connect_version_string != "": |
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.
I would make this one a if not connect_version_string
, I perceive it as slightly more robust as it will cover both "" and null.
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.
it should be if connect_version_string:
(no not
), right?
rsconnect/main.py
Outdated
) | ||
# 2024.01.1 or later, this can be removed. Requires access to the | ||
# Connect server version, which may be hidden. | ||
connect_version_string = ce.client.server_settings()["version"] |
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.
Should this maybe be .get("version")
? Or do we feel safe in taking for granted the version field will alwasy be there even when hidden?
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.
It will be there unless we make a breaking change in Connect, but at the same time I see no reason not to use the more robust code here.
7696d23
to
4868772
Compare
@christierney I think you can rebase again now and CI should be fixed |
4868772
to
30196b5
Compare
Intent
If Connect's server version is hidden, skip the starlette dependency check and log a warning.
Fixes #695
Type of Change
Approach
Only call
fix_starlette_requirements
if we have a non-empty Connect version string. Otherwise, log a warning, which will hopefully provide a helpful breadcrumb if users on a pre-2024.01.0 Connect see starlette errors.Automated Tests
Opted not to add a new test for such a small change.
Directions for Reviewers
I manually reproduced the bug by running Connect locally with
[Server] HideVersion = true
:logs of failed deploy
With this fix, the deployment succeeds:
logs of successful deploy
Checklist