Skip to content
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

ENH: include new ics vlans #3

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Dec 19, 2024

For reviewers: I'm mostly looking for a sanity check that these additional VLANs match reality.

The first commit here is a hotfix because Russ wanted to use the switchtool to set up some mfx-ics devices. Afterwards, I queried for the rest of the VLAN infos.

Then, I did the following:

I realize that the sorting of this file is really odd but I left it this way on purpose so it'd be easy to review. I'm all ears for suggestions on the best way to sort this, sorting by VLAN number and sorting by subnet name both feel strange to me.

Copy of Omar's comment from the Jira:

621 ics-abl.pcdns
613 ics-cxi.pcdsn
608 ics-det.pcdsn
620 ics-las.pcdsn
615 ics-mec.pcdsn
649 ics-mfx.pcdsn
631 ics-rix.pcdsn
644 ics-tmo.pcdsn
619 ics-txi-pcdsn
617 ics-ued.pcdsn
614 ics-xcs.pcdsn
647 ics-xpp.pcsn

tangkong
tangkong previously approved these changes Dec 19, 2024
Copy link

@tangkong tangkong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it matches omar's list, though I'm a bit unsure how to best check all of these

"631": "ics-rix.pcdsn",
"644": "ics-tmo.pcdsn",
"619": "ics-txi-pcdsn",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This -pcdsn is intentional but the sn <-> ns swaps are not? I realize it should be pcds, but what's the proper way to check these against our system? Are these just arbitrary names that we get to use or are they checked against some table?

Sincerely,
not a sysadmin

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed this one, will fix typo

Copy link

@tangkong tangkong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, from what I could see comparing to the ticket

@ZLLentz ZLLentz merged commit 6418a73 into pcdshub:master Dec 19, 2024
@ZLLentz ZLLentz deleted the enh_new_vlans branch December 19, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants