getting os details with /api/os #22

Merged
PeterSurda merged 1 commits from shailaja/idlers-agent:os_details into main 2024-07-07 09:36:20 +02:00
Member
No description provided.
shailaja added 1 commit 2024-07-01 16:44:19 +02:00
swapnil reviewed 2024-07-01 17:02:25 +02:00
agent.py Outdated
@ -353,0 +355,4 @@
def get_os_id(self):
try:
response = urllib.request.urlopen(f"{self.host}/api/os")
Member

This will not work. You should create a dedicated method in the ServerManager and use existing send_request.

This will not work. You should create a dedicated method in the ServerManager and use existing `send_request`.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-01 17:20:56 +02:00
agent.py Outdated
@ -353,0 +357,4 @@
try:
response = urllib.request.urlopen(f"{self.host}/api/os")
os_list = json.loads(response.read().decode())
current_os = os.uname().sysname.lower()
Member

os.uname().sysname provides the generic name of the operating system, not the specific distribution name or version.

For example, it would return 'Linux' for any Linux distribution (like Ubuntu, Fedora, etc.), 'Darwin' for macOS, and so on.

We should get complete name, for example Ubuntu 22.04.3 LTS in case of test2. We should read from /etc/os-release file.

`os.uname().sysname` provides the generic name of the operating system, not the specific distribution name or version. For example, it would return 'Linux' for any Linux distribution (like Ubuntu, Fedora, etc.), 'Darwin' for macOS, and so on. We should get complete name, for example `Ubuntu 22.04.3 LTS` in case of test2. We should read from `/etc/os-release` file.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-01 17:27:28 +02:00
agent.py Outdated
@ -353,0 +361,4 @@
for os_entry in os_list:
if current_os in os_entry['name'].lower():
return os_entry['id']
return next(os['id'] for os in os_list if os['name'].lower() == "other")
Member

Instead of directly falling to "other", maybe we can use id of ubuntu if exact name not found and if it's ubuntu, similarly for other OSes. For final fallback we can have "other" or "custom".

Instead of directly falling to "other", maybe we can use id of ubuntu if exact name not found and if it's ubuntu, similarly for other OSes. For final fallback we can have "other" or "custom".
swapnil marked this conversation as resolved
shailaja force-pushed os_details from 54f87b0314 to df525d2908 2024-07-02 09:08:24 +02:00 Compare
shailaja added 1 commit 2024-07-02 10:13:58 +02:00
shailaja force-pushed os_details from 276f29c707 to 04671ba669 2024-07-02 10:15:41 +02:00 Compare
shailaja added 1 commit 2024-07-02 20:15:52 +02:00
shailaja force-pushed os_details from 34e2a03db8 to 90dcb330a4 2024-07-02 20:17:47 +02:00 Compare
swapnil reviewed 2024-07-03 07:58:05 +02:00
agent.py Outdated
@ -240,0 +248,4 @@
if not os_info:
logging.error("No OS release info found.")
return next(os['id'] for os in os_list if os['name'].lower() == "other" or os['name'].lower() == "custom")
current_os = f"{os_info.get('NAME', 'Unknown')} {os_info.get('VERSION', '').strip()}".strip().lower()
Member

As discussed, we should use a combination of id and versionId. Check the exact key name in the file.

As discussed, we should use a combination of `id` and `versionId`. Check the exact key name in the file.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 08:02:27 +02:00
agent.py Outdated
@ -239,1 +236,3 @@
return os_id
def get_os_release_info(self):
try:
with open('/etc/os-release') as f:
Member

Since /etc/os-release doesn't exist on openwrt, we should fallback to /etc/VERSION.

As shared in the skype:

root@sysel:~# cat /etc/VERSION
majorversion="7"
minorversion="2"
major="7"
minor="2"
micro="1"
buildphase="GM"
buildnumber="69057"
smallfixnumber="5"
nano="5"
base="69057"
productversion="7.2.1"
os_name="DSM"
builddate="2024/03/28"
buildtime="17:49:10"

From /etc/VERSION, we can use a combination of os_name and productversion.

Since `/etc/os-release` doesn't exist on openwrt, we should fallback to `/etc/VERSION`. As shared in the skype: ```bash root@sysel:~# cat /etc/VERSION majorversion="7" minorversion="2" major="7" minor="2" micro="1" buildphase="GM" buildnumber="69057" smallfixnumber="5" nano="5" base="69057" productversion="7.2.1" os_name="DSM" builddate="2024/03/28" buildtime="17:49:10" ``` From `/etc/VERSION`, we can use a combination of `os_name` and `productversion`.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 08:03:21 +02:00
agent.py Outdated
@ -240,0 +261,4 @@
return next(os['id'] for os in os_list if os['name'].lower() == "other" or os['name'].lower() == "custom")
except Exception as e:
logging.error("Failed to fetch OS ID: {}".format(e))
return 27
Member

Instead of returning a hard-coded nnumber, we'll return "other" or "custom" id

Instead of returning a hard-coded nnumber, we'll return "other" or "custom" id
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 08:05:29 +02:00
agent.py Outdated
@ -408,0 +433,4 @@
def get_os_list(self):
try:
connection = http.client.HTTPSConnection(self.host)
Member

Again, we should use send_request here

Again, we should use `send_request` here
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 08:09:07 +02:00
agent.py Outdated
@ -240,2 +264,4 @@
return 27
def create_post_data(self):
Member

Missing parameter for os list

Missing parameter for os list
Member

Why is almost entry method is wrapped up in try-except? Please list down the cases where you think it'll break inside the try and this try-except will be useful.

Why is almost entry method is wrapped up in try-except? Please list down the cases where you think it'll break inside the `try` and this try-except will be useful.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 08:09:59 +02:00
agent.py Outdated
@ -243,3 +269,3 @@
post_data = {
"server_type": 1,
"os_id": self.get_os(),
"os_id": self.get_os(os_list),
Member

I think you mean get_os_id

I think you mean `get_os_id`
swapnil marked this conversation as resolved
shailaja added 1 commit 2024-07-03 11:49:26 +02:00
shailaja force-pushed os_details from 4a3222bb84 to 9c581fb6dd 2024-07-03 11:52:43 +02:00 Compare
swapnil reviewed 2024-07-03 15:43:15 +02:00
agent.py Outdated
@ -240,0 +271,4 @@
for os_entry in os_list:
if os_entry['name'].lower() in ["other", "custom"]:
return os_entry['id']
return None
Member

os id cannot be none, so at least this method should never return a None. Also, this case should never practically happen, even if it happens, you can return 1.

os id cannot be none, so at least this method should never return a None. Also, this case should never practically happen, even if it happens, you can return 1.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 15:43:54 +02:00
agent.py Outdated
@ -240,0 +274,4 @@
return None
if 'NAME' in os_info and 'VERSION' in os_info:
current_os = f"{os_info.get('NAME', 'Unknown')} {os_info.get('VERSION', '').strip()}".strip().lower()
Member

It's still not using id and versionId

It's still not using `id` and `versionId`
Member

@shailaja We want to use ID and VERSION_ID instead of NAME and VERSION, then you don't need to lower() that.

@shailaja We want to use `ID` and `VERSION_ID` instead of `NAME` and `VERSION`, then you don't need to `lower()` that.
Member

You also need to modify this line to use ID and VERSION_ID.

current_os = f"{os_info.get('NAME', 'Unknown')} {os_info.get('VERSION', '').strip()}".strip().lower()
You also need to modify this line to use ID and VERSION_ID. ```python current_os = f"{os_info.get('NAME', 'Unknown')} {os_info.get('VERSION', '').strip()}".strip().lower() ```
Member

Also we should refrain from using f-string anywhere in the new code.

Also we should refrain from using f-string anywhere in the new code.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 15:44:36 +02:00
agent.py Outdated
@ -240,2 +304,4 @@
return None
def create_post_data(self):
Member

Missing parameter for os list.

Missing parameter for os list.
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-03 15:46:08 +02:00
agent.py Outdated
@ -407,0 +473,4 @@
def get_os_list(self):
os_list = self.send_request('GET', '/api/v1/os')
if os_list:
Member

Don't need to do if-else. can be one-liner like other get_* methods.

Don't need to do if-else. can be one-liner like other `get_*` methods.
swapnil marked this conversation as resolved
shailaja added 1 commit 2024-07-05 11:35:00 +02:00
shailaja force-pushed os_details from 49a3f63c34 to 777d742d40 2024-07-05 11:38:49 +02:00 Compare
swapnil reviewed 2024-07-05 14:50:37 +02:00
agent.py Outdated
@ -240,3 +301,4 @@
def create_post_data(self):
ram, disk = self.get_ram_and_disk()
os_list = self.get_os_list()
Member

get_os_list method does not exist in the ServerData class. You need to take os_list as an argument to create_post_data

`get_os_list` method does not exist in the `ServerData` class. You need to take `os_list` as an argument to `create_post_data`
swapnil marked this conversation as resolved
swapnil reviewed 2024-07-05 14:56:04 +02:00
agent.py Outdated
@ -406,1 +468,4 @@
return self.create_note(note_data)
def get_os_list(self):
os_list = self.send_request('GET', '/api/v1/os')
Member

The correct endpoint is /api/os and not /api/v1/os

The correct endpoint is `/api/os` and not `/api/v1/os`
swapnil marked this conversation as resolved
shailaja added 1 commit 2024-07-05 15:25:49 +02:00
shailaja force-pushed os_details from 334ce0aefd to 5316b0fb07 2024-07-05 15:29:37 +02:00 Compare
shailaja added 1 commit 2024-07-05 16:05:48 +02:00
shailaja force-pushed os_details from 4480a3361f to eec2530205 2024-07-05 16:07:35 +02:00 Compare
swapnil approved these changes 2024-07-05 16:15:19 +02:00
swapnil reviewed 2024-07-05 16:17:28 +02:00
agent.py Outdated
@ -242,0 +276,4 @@
if 'ID' in os_info and 'VERSION_ID' in os_info:
current_os = (os_info.get('ID', 'Unknown') + " " + os_info.get('VERSION_ID', '').strip()).strip()
else:
current_os = (os_info.get('os_name', 'Unknown') +" " os_info.get('productversion', '').strip()).strip().lower()
Member

You missed a + after " "

You missed a `+` after `" "`
swapnil marked this conversation as resolved
shailaja added 1 commit 2024-07-05 16:19:37 +02:00
shailaja force-pushed os_details from 31ee586a9b to deac2cdee2 2024-07-05 16:21:34 +02:00 Compare
swapnil reviewed 2024-07-05 19:15:15 +02:00
agent.py Outdated
@ -428,2 +494,4 @@
logging.info('Server id: {}'.format(server_id))
os_list = server_manager.get_os_list()
post_data = server_data.create_post_data(os_list)
Member

Why this call again? There's already a call for create_post_data above, you need to modify that.

Why this call again? There's already a call for create_post_data above, you need to modify that.
shailaja force-pushed os_details from deac2cdee2 to d6afa1c5eb 2024-07-05 20:00:07 +02:00 Compare
shailaja added 1 commit 2024-07-05 20:03:50 +02:00
shailaja force-pushed os_details from 4fa1281c10 to 8253e5fc51 2024-07-05 20:09:38 +02:00 Compare
PeterSurda merged commit 8253e5fc51 into main 2024-07-07 09:36:20 +02:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Sysdeploy/idlers-agent#22
No description provided.