Closed Bug 659157 Opened 10 years ago Closed 10 years ago

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

Categories

(Cloud Services :: Server: Core, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: telliott, Assigned: telliott)

References

Details

Attachments

(1 file, 1 obsolete file)

Blocks: 659158
Duplicate of this bug: 631166
No longer blocks: 659158
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)
Attachment #544914 - Flags: review?(jrconlin) → review?(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
Attachment #544914 - Flags: review?(tarek)
(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.
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 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+
Oooh, decorators. I should totally do that. 

For now all in http://hg.mozilla.org/services/server-node-assignment/rev/281229c2f81c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 676423
You need to log in before you can comment on or make changes to this bug.