Conversation
|
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
|
Verified the fix When you have existing entry with server name null
Name is now required arg |
ChaitaC
left a comment
There was a problem hiding this comment.
I verified that the PR resolves the initial issue, and all the changes look good to me.
However, since we’re making the name argument required, I’ll leave the final approval to someone more familiar with this part of the code. I can’t think of any scenarios where this change would cause issues.
Do we have any older documentation that explicitly mentioned name as optional? I did a quick check but didn’t find anything.
|
tests against latest version of Connect: https://github.com/posit-dev/connect/actions/runs/19311989599/job/55234313069 |
4f49f5b to
e379b83
Compare
tdstein
left a comment
There was a problem hiding this comment.
What happens if multiple entries in servers.json have a value of "None"?
|
toph-allen
left a comment
There was a problem hiding this comment.
Do you think it's worth adding a server without a name to the tests for list? You would just need to add one without a nickname around line 25, it would be pretty easy.
I manually verified this locally, but it's better to also have a test probably.
I'll approve this now, as functionally it works — better if you add a short test that list works with a no-name server before you merge.
Ok, just wanted to ensure that it didn't blow up if multiple entries have a name of |
Intent
Resolves #705
Type of Change
Approach
rsconnect listfrom being fed a null name. This will provide a fix for any users with currently "corrupted"servers.jsonfiles.Make--namea required param forrsconnect add. To me there is no use case forrsconnect add-ing a server with no name as you wouldn't be able to reference it in other commands (like deploy).Validation
rsconnect listChecklist