659157 - Spin node assignment off into it's own api and libs

Status: RESOLVED FIXED

(Cloud Services :: Server: Core, defect)

Description

[Toby Elliott [:telliott]]

Documented at https://wiki.mozilla.org/Services/NodeAssignment

Comment 2

[Toby Elliott [:telliott]]

Attachment: the new implementation of the specified API, including test cases

The build may still be a little off, but the whole thing works and passes all the tests, so yay.

Comment 3

[Tarek Ziadé (:tarek)]

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

Comment 4

[Toby Elliott [:telliott]]

(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.

Comment 5

[Toby Elliott [:telliott]]

Attachment: Completely new node assignment

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.

Comment 6

[Rob Miller [:rmiller]]

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?

Comment 7

[Toby Elliott [:telliott]]

Oooh, decorators. I should totally do that. 

For now all in http://hg.mozilla.org/services/server-node-assignment/rev/281229c2f81c