REF: netconfig to sdfconfig for the gui #8
Conversation
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
I think the changes here look reasonable. Left some comments/food for thought.
There's certainly more to do to make this more maintainable, particularly removing vestigial code and more thoroughly type hinting. But that's for future us
| - Use sdfconfig instead of netconfig, which has been | ||
| deprecated and scheduled for removal. |
There was a problem hiding this comment.
It's funny to see this line under "Maintenance", since it's such a pervasive change. I don't think there's a better place for it, to be fair.
There was a problem hiding this comment.
At least there's related notes all over the api breaks section
|
Did vscode eat all my comments >:V |
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
Testing a single comment
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
I think I covered the comments lost to the abyss of vscode 😢
I think the changes here are reasonable, and do what you set out to do. There's more to go in making this more maintainable, but that's for a separate effort.
Co-authored-by: Robert Tang-Kong <35379409+tangkong@users.noreply.github.com>
|
I found one more tiny issue while retesting- one case where the user can be told to configure their sdfconfig but it is configured correctly. I'm seeing if I can resolve it and move back to other things. |
|
I think this is stable now- probably still letting it sit until team members can use sdfconfig. |
|
@pcdshub/python-reviewers this is ready for final review as users will start getting sdfconfig/foreman access this week |
tangkong
left a comment
tangkong
left a comment
There was a problem hiding this comment.
This looks good to me on the whole, one small question but I think we could possibly ignore it
| import json | ||
|
|
||
|
|
||
| @functools.lru_cache(maxsize=1000) |
There was a problem hiding this comment.
If we modify an entry with switch tool, will this cache force us into displaying stale information? This is speaking to my lack of experience with the app, sorry if this is off-base
There was a problem hiding this comment.
This only returns information that could be modified by sdfconfig/foreman, not within switchtool itself.
The rationale here is that we don't change the hostname <-> mac addr mapping often, so we can save a lot of time by caching.
2 participants
Description
Switch out all usages of netconfig for an equivalent usage in sdfconfig if it is used in the switchtool gui.
Also add a --switch-type command-line argument so you can startup the tool without a description field, which is currently dropped in the transition from netconfig to sdfconfig.
Note that the subnet names changed between the two tools:
cds-xcs.pcdsnPCDSN-CDS-XCSIt's possibly I didn't get all of these correct.
In cases where I made major edits to a file, or where I was trying to figure out if I needed to make edits, but couldn't grok it with the lack of type hints, etc., I also took the time to unravel all ruff and pylance warnings for those files.
In cases where I didn't really need to edit a file, but the fix was super obvious and quick, I did so.
Note that there are lingering code branches that I have left broken because they aren't used in the gui. These parts still rely on netconfig. I disabled some of these functions to check if the gui was using them. If someone wants they could fix those parts later.
Motivation and Context
Netconfig is going away and switchtool uses it a lot.
How Has This Been Tested?
Interactively only- seems OK, needs more testing probably.
Where Has This Been Documented?
Here and in jira only
Pre-merge checklist
New/changed functions and methods are covered in the test suite where possibleTest suite passes locallyTest suite passes on GitHub Actionsdocs/pre-release-notes.shand created a pre-release documentation page