Closed
Bug 659157
Opened 14 years ago
Closed 14 years ago
Spin node assignment off into it's own api and libs
Categories
(Cloud Services :: Server: Core, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: telliott, Assigned: telliott)
References
Details
Attachments
(1 file, 1 obsolete file)
83.42 KB,
patch
|
rmiller
:
review+
|
Details | Diff | Splinter Review |
Documented at https://wiki.mozilla.org/Services/NodeAssignment
Assignee | ||
Comment 2•14 years ago
|
||
The build may still be a little off, but the whole thing works and passes all the tests, so yay.
Assignee: nobody → telliott
Attachment #544914 -
Flags: review?(jrconlin)
Assignee | ||
Updated•14 years ago
|
Attachment #544914 -
Flags: review?(jrconlin) → review?(tarek)
Comment 3•14 years ago
|
||
Comment on attachment 544914 [details] [diff] [review]
the new implementation of the specified API, including test cases
I don't understand how this code works.
For example you do in the constructor:
try:
self.nodes = load_and_configure(app.config, 'nodes')
except Exception:
logger.error(traceback.format_exc())
logger.error("unable to load nodes. Problem?")
then you use self.nodes elsewhere but it was not initialized
Also, you are using sql expressions here, and it seems like you wanted to get rid of those for having a clearer view, so..
My personal opinion is that if this code works, it's not tested enough, but I don't want to sound rude here -- so maybe I should not be the reviewer on that one
Updated•14 years ago
|
Attachment #544914 -
Flags: review?(tarek)
Assignee | ||
Comment 4•14 years ago
|
||
(In reply to comment #3)
> Comment on attachment 544914 [details] [diff] [review] [review]
> the new implementation of the specified API, including test cases
>
> I don't understand how this code works.
>
> For example you do in the constructor:
>
> try:
> self.nodes = load_and_configure(app.config, 'nodes')
> except Exception:
> logger.error(traceback.format_exc())
> logger.error("unable to load nodes. Problem?")
>
> then you use self.nodes elsewhere but it was not initialized
>
Good catch. Need to explicitly set it to None there.
> Also, you are using sql expressions here, and it seems like you wanted to
> get rid of those for having a clearer view, so..
>
Yep. This was written several weeks ago, before we discussed using SQL expressions in the code.
Assignee | ||
Comment 5•14 years ago
|
||
Includes removing all the old version, which we aren't building to at this point, so don't worry about all the deletes.
I can't make RPMs from this yet, I believe due to the problem I describe in 672655, but I can make the app and it passes all tests.
Attachment #544914 -
Attachment is obsolete: true
Attachment #549271 -
Flags: review?(rmiller)
Comment 6•14 years ago
|
||
Comment on attachment 549271 [details] [diff] [review]
Completely new node assignment
Review of attachment 549271 [details] [diff] [review]:
-----------------------------------------------------------------
::: nodes/admin.py
@@ +57,5 @@
> + self.admin = self.app.config.get('nodes.admin')
> +
> + def get_cluster_list(self, request):
> + if self.admin is not True:
> + return HTTPNotFound()
This check is at the start of each controller method. Maybe worth using an @admin_required decorator?
Attachment #549271 -
Flags: review?(rmiller) → review+
Assignee | ||
Comment 7•14 years ago
|
||
Oooh, decorators. I should totally do that.
For now all in http://hg.mozilla.org/services/server-node-assignment/rev/281229c2f81c
Assignee | ||
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•