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.

Sign up for GitHub

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

Jump to bottom

Add OS info to LXD pods in pod list. #1061

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Add OS info to LXD pods in pod list. #1061

merged 2 commits into from
Apr 30, 2020

Conversation

Caleb-Ellis
Copy link
Contributor

Done

  • Added OS info to LXD pods in pod list

QA

  • Go to the pod list and check that LXD pods show the OS info, and the KVM pods should show "Unknown"

Fixes

Fixes #1003

Screenshot

Screenshot_2020-04-30 KVM bolla MAAS

@Caleb-Ellis Caleb-Ellis force-pushed the pod-os-info branch 2 times, most recently from dcfafd2 to be90440 Compare April 30, 2020 00:49
squidsoup
squidsoup approved these changes Apr 30, 2020
View reviewed changes
Copy link
Contributor

@squidsoup squidsoup left a comment

Choose a reason for hiding this comment

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

Looks good, a few small suggestions for your consideration.

legacy/src/app/controllers/pods_list.js Outdated
return items.find(item => item.id === itemId).name;
$scope.getOSInfo = (pod) => {
const podHost = $scope.hostMap.get(pod.id);
if (podHost) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to move this host parsing code to a reusable function somewhere that takes osystem, distro_series and releases as arguments? Imagine we'll want to use this when we rebuild this view in react.

Copy link
Contributor Author

@Caleb-Ellis Caleb-Ellis Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good idea. I've separated out a function that takes node and osInfo as arguments, where osInfo is the data returned from the general.osinfo websocket handler.

Come to think of it we probably already have a function in the react codebase that handles this, because we need it for the Status column in the machine list.

legacy/src/app/controllers/pods_list.js Outdated

return items.find(item => item.id === itemId).name;
$scope.getOSInfo = (pod) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name seems a bit misleading, as we get osinfo (which is a complex object) on l70, when this is really returning an "OS shortname" or something along those lines.

@Caleb-Ellis Caleb-Ellis merged commit 2e6f9e3 into canonical:master Apr 30, 2020
@Caleb-Ellis Caleb-Ellis deleted the pod-os-info branch April 30, 2020 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squidsoup squidsoup squidsoup approved these changes

Assignees
No one assigned
Labels
Review: Code +1 Review: QA +1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add OS info to LXD pods in list
2 participants
@Caleb-Ellis @squidsoup