socallinuxexpo / scale-network

SCaLE's on-site expo network configurations, wifi, tooling, and scripts
https://www.socallinuxexpo.org/
BSD 3-Clause "New" or "Revised" License
40 stars 16 forks source link

Make new inventory.py pass pylint #591

Closed kylerisse closed 8 months ago

kylerisse commented 1 year ago

Description

In order to quickly iterate on the NixOS migration, @sarcasticadmin added https://github.com/socallinuxexpo/scale-network/blob/ffda5117e0fd7757dbf4d43f190f13f5092b0254/facts/inventory.py#L2 . Now that we're between shows again, lets clean it up.

Acceptance Criteria

kylerisse commented 10 months ago

@owendelong I see this file /ansible/LibInventory.pm in the repo. It looks unused and commit suggests it was for KEA. Since we're configuring Kea with Nix, I don't believe we need this file. Can you please confirm? Only reason I ask is that @sarcasticadmin and I are going to be 1. removing this /ansible directory soon and 2. modifying the data structure that inventory.py produces. I want to make sure I don't break anything you have integrated or planned. Thank you!

owendelong commented 10 months ago

I wasn’t involved in our Kea implementation before or with NIX, so actually, Rob is the one to ask this, primarily.

I think this was intended to make the inventory.py output accessible to PERL scripts. As long as the output is In JSON, this perl script is data agnostic (it’s super simple and basically a wrapper around decode_json() from The JSON.PM package.

I don’t see anything using it: delong-dhcp162:owen (128) ~/SCALE/networking/scale_repo/scale-network % grep -ri LibInventory . Binary file ./.git/objects/pack/pack-321bec2ab4c7e511bcd2176526df353c12c58a1e.pack matches Binary file ./.git/index matches

(That’s against current Master with some slight tweaks I made to LibInventory.pm to check it): delong-dhcp162:owen (129) ~/SCALE/networking/scale_repo/scale-network % git status On branch master Your branch is up to date with 'origin/master'.

Changes not staged for commit: (use "git add ..." to update what will be committed) (use "git restore ..." to discard changes in working directory) modified: ansible/LibInventory.pm

no changes added to commit (use "git add" and/or "git commit -a")

delong-dhcp162:owen (130) ~/SCALE/networking/scale_repo/scale-network % git diff diff --git a/ansible/LibInventory.pm b/ansible/LibInventory.pm index 00d353f..68d2c61 100755 --- a/ansible/LibInventory.pm +++ b/ansible/LibInventory.pm @@ -4,8 +4,9 @@ # use strict;

-#use JSON; +use JSON; use Data::Dumper; +main();

sub main { @@ -24,6 +25,6 @@ sub main sub JSONParse { my $string = shift @; -# return(decode_json($string));

So I think you’re free to remove the directory. I would like to keep the script around because it provides a convenient way to produce a human readable version of what’s inventory.py produces. I’ll go ahead and add this to a PR.

PR submitted and ready for review.

Owen

On Sep 21, 2023, at 11:18, kylerisse @.***> wrote:

@owendelong https://github.com/owendelong I see this file /ansible/LibInventory.pm https://github.com/socallinuxexpo/scale-network/blob/master/ansible/LibInventory.pm in the repo. It looks unused and commit suggests it was for KEA. Since we're configuring Kea with Nix, I don't believe we need this file. Can you please confirm? Only reason I ask is that @sarcasticadmin https://github.com/sarcasticadmin and I are going to be 1. removing this /ansible directory soon and 2. modifying the data structure that inventory.py https://github.com/socallinuxexpo/scale-network/blob/master/facts/inventory.py produces. I want to make sure I don't break anything you have integrated or planned. Thank you!

— Reply to this email directly, view it on GitHub https://github.com/socallinuxexpo/scale-network/issues/591#issuecomment-1730079601, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK6GTRA567RWDJHEK6H52TX3SAILANCNFSM6AAAAAAVZQ7UCM. You are receiving this because you were mentioned.

kylerisse commented 10 months ago

Cool, yea all the Nix KEA stuff is strait forward enough. This commit made it look like you had intended to do Kea stuff with perl prior to the Nix effort, only reason I asked. I had already grepped around. We’ll add human readable output as part of the new work. We’ll get it all squared away next week. Thanks Owen!

nixinator commented 10 months ago

not sure if this relevant, but nix can output to JSON.

https://github.com/NixOS/nix/commit/8abb80a478116b10bf37162c71f602262de412a9

might be interest to capture this data, and do something with it, like comparing a machine against a baseline or a single source of truth.

owendelong commented 8 months ago

This topic has wandered a bit, but IIRC, inventory.py is now deprecated and the core issues discussed have been resolved. If anyone believes this is wrong, please re-open this issue.