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

Adds support for listing tags, filtering machines by tag, and getting machine hardware info #96

Merged
merged 6 commits into from
Jun 21, 2022
Merged

Adds support for listing tags, filtering machines by tag, and getting machine hardware info #96

merged 6 commits into from
Jun 21, 2022

Conversation

markylaing
Copy link
Contributor

  • Adds a new interface Tag (implemented by tag in tags.go) for interacting with MAAS tags.
  • Adds a method Tags() ([]Tag, error) to the Controller for retrieving a slice of MAAS tags.
  • Adds Tags to MachineArgs to allow filtering machines by tag.
  • Adds a method HardwareInfo() map[string]string to the Machine interface and implements it. Hardware info is added to machine when deserializing the API response.

I have tried to follow conventions that I have found elsewhere in the codebase but let me know if I have missed something, thanks 😄

@jujubot
Copy link
Contributor

jujubot commented May 26, 2022

Can one of the admins verify this patch?

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented May 26, 2022

Can one of the admins verify this patch?

@markylaing markylaing force-pushed the tags-hardware-info branch from d4c1c87 to 7819ada Compare May 26, 2022 13:11
stgraber
stgraber reviewed May 26, 2022
View reviewed changes
tags.go Outdated Show resolved Hide resolved
@markylaing markylaing force-pushed the tags-hardware-info branch from 7819ada to 6bcc018 Compare May 26, 2022 15:21
@jameinel
Copy link
Member

Tags / MachineArgs and HardwareInfo seem relatively straightforward, but it is a bit of a surprise that you're also changing how dispatchSingleRequest (and adding an HTTPClient override), without document the what and why you're changing it in the PR overview.
(It also seems weird to me that dispatchSingleRequest is not a pointer receiver and spawns an http.Client for every request, rather than allowing connection sharing.)
can you elaborate a bit on those changes?

@stgraber
Copy link
Contributor

@markylaing is out for a few days but I agree that it'd make sense to clarify the need for the http.Client in the PR or maybe even split this into two PRs.

The reason why we want to have a custom http.Client is so we can do proper TLS validation of otherwise self-signed servers as well as more tightly control timeouts.

jameinel
jameinel reviewed May 31, 2022
View reviewed changes
Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

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

If you did want to expose httpClient, we would definitely want to add some tests that it is supported/being used when appropriate.

machine.go Outdated Show resolved Hide resolved
machine.go Outdated Show resolved Hide resolved
tags.go Show resolved Hide resolved
tags.go Show resolved Hide resolved
@markylaing
Copy link
Contributor Author

@markylaing is out for a few days but I agree that it'd make sense to clarify the need for the http.Client in the PR or maybe even split this into two PRs.

The reason why we want to have a custom http.Client is so we can do proper TLS validation of otherwise self-signed servers as well as more tightly control timeouts.

@jameinel @stgraber Ok I'll split this into two PRs.

@markylaing markylaing force-pushed the tags-hardware-info branch from 6bcc018 to 88024ad Compare June 9, 2022 10:43
@markylaing
tags: Adds and implements Tag interface.
d9fe04e 
Adds required methods for reading a list of tags.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing markylaing force-pushed the tags-hardware-info branch from 88024ad to ef64133 Compare June 9, 2022 11:18
@markylaing
Copy link
Contributor Author

@jameinel @stgraber I think I've addressed all of your comments. Here is a link to the second PR containing the commits which allow a configurable http client: #97

@markylaing markylaing requested a review from jameinel June 9, 2022 15:03
jameinel
jameinel approved these changes Jun 16, 2022
View reviewed changes
tags.go Show resolved Hide resolved
controller.go Outdated Show resolved Hide resolved
@markylaing
controller: Adds and implements Tags method on controller.
82e1191 
Adds method to controller interface for listing all MAAS tags.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
controller: Filter machines by tag.
f03801e 
Adds Tags field to MachinesArgs and adds these values to query parameters
to filter results.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
machine: Adds and implements HardwareInfo on Machine interface.
cd944ef 
Adds methods for retrieval of hardware info from the MAAS API response
for a machine.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
machine: Adds hardware info to test data.
b3857c8 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
tags: Adds tests for reading tags.
16207a0 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing markylaing force-pushed the tags-hardware-info branch from bb9eecd to 16207a0 Compare June 16, 2022 08:40
@jameinel
Copy link
Member

/build

@jameinel
Copy link
Member

Failure on this build was because of a misconfiguration of Jenkins. I'm looking to fix that and will resubmit.

@markylaing
Copy link
Contributor Author

@jameinel Did you have any success fixing the Jenkins configuration?

@jameinel
Copy link
Member

jameinel commented Jun 21, 2022 via email

@markylaing
Copy link
Contributor Author

@jameinel no worries 👍

@jameinel
Copy link
Member

/build

1 similar comment
@jameinel
Copy link
Member

/build

@jameinel
Copy link
Member

It has been fixed, and the build check passed. So I'll merge.

@jameinel
Copy link
Member

/merge

@jameinel
Copy link
Member

/merge

@jujubot jujubot merged commit 7268ed0 into juju:v2 Jun 21, 2022
@markylaing
Copy link
Contributor Author

It has been fixed, and the build check passed. So I'll merge.

@jameinel thank you!

SimonRichardson added a commit to SimonRichardson/gomaasapi that referenced this pull request Mar 16, 2023
@SimonRichardson
Optional HardwareInfo
ac1f1af 
Unfortunately, HardwareInfo isn't always available for every MAAS
version. The prior PR[1] expected that hardware_info was available. The
fix is simple, just make it optional when attempting to validate it with
the schema coercing.

Additionally, I've added a few more tests to ensure that we're testing
various scenarios of hardware_info being omitted. Lastly, it felt like
there was a missing check for TestReadMachines for validating the
hardware info for a given machine response.

This should fix[2] in Juju.

 1. juju#96
 2. https://bugs.launchpad.net/juju/+bug/2009064
@SimonRichardson SimonRichardson mentioned this pull request Mar 16, 2023
Merged
jujubot added a commit that referenced this pull request Mar 17, 2023
@jujubot
Merge pull request #99 from SimonRichardson/optional-hardware-info
5277155 
#99

Unfortunately, HardwareInfo isn't always available for every MAAS version. The prior PR[1] expected that hardware_info was available. The fix is simple, just make it optional when attempting to validate it with the schema coercing.

Additionally, I've added a few more tests to ensure that we're testing various scenarios of hardware_info being omitted. Lastly, it felt like there was a missing check for TestReadMachines for validating the hardware info for a given machine response.

This should fix[2] in Juju.

 1. #96
 2. https://bugs.launchpad.net/juju/+bug/2009064
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stgraber stgraber stgraber left review comments

@jameinel jameinel jameinel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@markylaing @jujubot @jameinel @stgraber