Closed Bug 984721 Opened 8 years ago Closed 8 years ago

Slaveapi should abstract out Machine from Slave

Categories

(Release Engineering :: General, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Callek, Assigned: Callek)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch [slaveapi] v1 (obsolete) — Splinter Review
This does the basics, as suggested by ben

There is room for improvement here, e.g.

Machine
 \-PDU
 |-OOBInterface
 |-Slave
 |-BuildbotSlave
 |-ImagingServer
 |-... etc

But this patch only deals with [Buildbot]Slave
Attachment #8392650 - Flags: review?(bhearsum)
Comment on attachment 8392650 [details] [diff] [review]
[slaveapi] v1

Review of attachment 8392650 [details] [diff] [review]:
-----------------------------------------------------------------

::: slaveapi/slave.py
@@ -172,5 @@
>      # e.g. a foopy
> -    def load_all_info(self):
> -        self.load_inventory_info()
> -        self.load_ipmi_info()
> -        self.load_bug_info()

Just calling this out here, this change does remove "bug info" from foopies, but we're not actually using it anywhere right now. So while foopies can have their own problem tracking bugs, they rarely do and we have no need to create them here either.
Comment on attachment 8392650 [details] [diff] [review]
[slaveapi] v1

Review of attachment 8392650 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly fine, just a couple of things:

::: slaveapi/clients/machine.py
@@ +1,1 @@
>  import time

I'd been thinking of the clients directory as code that deals with external services, which this doesn't fit into. I'm OK with this if you feel strongly, though.

@@ -206,5 @@
>  
>      # Then wait for it come back up.
> -    return is_alive(slave, timeout=alive_timeout)
> -
> -def get_console(slave, usebuildbotslave=False):

A console seems like something that probably belongs with Machine. If you associate this with a Slave, you'll need to do the same with all the subclasses in the future.

::: slaveapi/slave.py
@@ +120,2 @@
>      # e.g. a foopy
> +    pass

I don't see any reason for this to exist. Why not just s/BuildbotSlave/Machine/ in consuming code?
Attachment #8392650 - Flags: review?(bhearsum) → feedback+
Attached patch [slaveapi] v2 (obsolete) — Splinter Review
Since v1:

 * Moved machine to a new folder (machines) and named the file "base"
 * Left BuildbotSlave class alone as per IRC discussion
 * Left get_console alone as per jhopkin's preference
Attachment #8392650 - Attachment is obsolete: true
Attachment #8394515 - Flags: review?(bhearsum)
Attached patch [slaveapi] v2.1Splinter Review
forgot to adjust some import paths in v2, hence v2.1
Attachment #8394515 - Attachment is obsolete: true
Attachment #8394515 - Flags: review?(bhearsum)
Attachment #8394530 - Flags: review?(bhearsum)
Attachment #8394530 - Flags: review?(bhearsum) → review+
Comment on attachment 8394530 [details] [diff] [review]
[slaveapi] v2.1

$ git push
Counting objects: 42, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (29/29), done.
Writing objects: 100% (29/29), 4.22 KiB | 0 bytes/s, done.
Total 29 (delta 22), reused 0 (delta 0)
To ssh://git.mozilla.org/build/slaveapi.git
   1173310..ccce02a  master -> master

$ git log 1173310..ccce02a
commit ccce02ac8aa2bd7a000733a8d1211881d84fd927
Author: Justin Wood <Callek@gmail.com>
Date:   Tue Mar 25 22:46:10 2014 -0400

    Bump slaveapi ver for Bug 921067

commit ce5eb8a162100f825b910a92c43cc7f6cb10d8e5
Author: Justin Wood <Callek@gmail.com>
Date:   Tue Mar 25 22:44:17 2014 -0400

    Bug 921067 - Slaveapi should support panda devices for reboots. r=bhearsum

commit 8d13f0e58ce69436973a25ee2209cc09c08cf24a
Author: Justin Wood <Callek@gmail.com>
Date:   Tue Mar 25 22:41:45 2014 -0400

    Bug 984721 - Slaveapi should abstract out Machine from Slave. r=bhearsum

commit 7b3966fd3b2904a9b51edac38239bba432ac16cc
Author: Justin Wood <Callek@gmail.com>
Date:   Tue Mar 25 22:35:30 2014 -0400

    Bug 981039 - slaveapi should have a seperate knob for concurrency on buildapi calls alone. r=bhearsum
Attachment #8394530 - Flags: checked-in+
had to re-bump the version for slaveapi due to a mistake here.

Needed to add slaveapi.machines to packages list in setup.py
deployed for real as part of current Slaveapi 1.0.18.1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Component: Tools → General
You need to log in before you can comment on or make changes to this bug.