-
Notifications
You must be signed in to change notification settings - Fork 7
MNT: sdfconfig #35
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
base: master
Are you sure you want to change the base?
MNT: sdfconfig #35
Conversation
…re on the path during the test suite. This is how we specify them in prod now but this test was never checked or updated.
tangkong
left a comment
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.
The changes here seem good, I don't have access to sdfconfig so my interactive testing is somewhat limited.
iocmanager loads fine for me, with "please configure sdfconfig" all over. I might have put that string in brackets or something to make it stand out, but other than that this looks good.
I'll hold off on the approve until we get verification that the rest of the team can auth with sdfconfig
| ) | ||
|
|
||
|
|
||
| def sdfconfig(host: str, domain: str = "pcdsn") -> dict[str, str]: |
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.
(not really an issue to be fixed in this PR)
I wonder how much the overhead of calling sdfconfig is. Before we called netconfig and just ingested everything, and while this is certainly more efficient for single hosts I do wonder what happens if we end up needing to query lots of hosts.
Does sdfconfig have an option for looking up information on multiple hosts at once?
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.
netconfig was also used one host at a time here. sdfconfig has a search option too but the hosts would need to have some similarity.
The overhead is more than I would like, but for iocmanager at least we do this on a background thread to not pause the app execution.
| Parameters | ||
| ---------- | ||
| host : str | ||
| The hostname |
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.
Nitpicky, but domain is also parameter
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 catch
iocmanager/server_tools.py
Outdated
| or an empty dict if there was no information. | ||
| """ | ||
| output = {} | ||
| pattern = re.compile(r"^(\S.+?):\s+(\S.+)$") |
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.
After mulling it over, I think it's worth switching this back to simple text parsing instead of regex due to readability concerns.
…e replaced by metadata field
…standing of the plan
Do not merge/tag until team members can access
sdfconfig.Use
sdfconfiginstead ofnetconfigto get the "location" and "description" information about servers as we click through the IOC table.https://jira.slac.stanford.edu/browse/ECS-9486
Netconfig is being deprecated/disabled/removed soon, so we need to switch.
Interesting decisions made:
--legacyswitch because the output was different enough to break the old parsing code anyway. (I've also requested a json output which would make the parsing code less silly).I tested this in the following way: