Merge lp://staging/~salvatore-orlando/neutron/quantum-api into lp://staging/neutron/diablo

1) We shouldn't be checking in .project .pydevproject and .pydevproject.moved files.
2) pep8.errors seems out of date, I merged Santhosh and Rajaram's pep8 fixes branch and trunk is pep8 clean.
3) I got the branch and there are 2 minor pep8 issues:

./tests/functional/test_service.py:52:1: W293 blank line contains whitespace
./tests/functional/test_service.py:54:5: E303 too many blank lines (2)

4) I would be ok with leaving the debug messages in, if the user doesn't want the debug messages they could change the log level.

5) Based on the API spec. The accepted values for port-sate are:
   * ACTIVE
   * DOWN
So, we should update that to be consistent, currently the code uses values "UP" and "DOWN" at some places.

6) In openvswitch/README , the change from quantum-framework -> quantum- should be "quantum" no dash at end.

7) The trunk doesn't have the following spurious methods "get_interface_details" and "get_all_attached_interfaces" so this branch should merge trunk changes.

8) I would be ok with delaying the merge till unit tests are there would be good by me, if thats less work for you.

Hi Somik,
thanks for your review!

The only point which probably deserves a bit of discussion is #7. Please let me know what you think of my proposal.

> 1) We shouldn't be checking in .project .pydevproject and .pydevproject.moved
> files.
I should have messed up the commit operation. I will remove them from source control.

> 2) pep8.errors seems out of date, I merged Santhosh and Rajaram's pep8 fixes
> branch and trunk is pep8 clean.

If you are talking about the pep8.errors file this shouldn't be there just as .project and the other .* files. It is a file which I generate with a script before commit.

> 3) I got the branch and there are 2 minor pep8 issues:
>
> ./tests/functional/test_service.py:52:1: W293 blank line contains whitespace
> ./tests/functional/test_service.py:54:5: E303 too many blank lines (2)
>

The script I was talking about needs to be improved... I copied it from my nova development environment, where the tests directory is under nova. This means I did not checked pep8 errors in tests before commit.

> 4) I would be ok with leaving the debug messages in, if the user doesn't want
> the debug messages they could change the log level.
>

No, it is not ok to leave the debug messages, especially if they are prints and not LOG.debug. I thought I had removed all the unnecessary ones.

> 5) Based on the API spec. The accepted values for port-sate are:
> * ACTIVE
> * DOWN
> So, we should update that to be consistent, currently the code uses values
> "UP" and "DOWN" at some places.
>

Rigth. Should we use an enumeration in place of the hardcoded strings?

> 6) In openvswitch/README , the change from quantum-framework -> quantum-
> should be "quantum" no dash at end.
>

I don't know how that file got changed. I will revert the changes in it, as I did not want to make any change :-)

> 7) The trunk doesn't have the following spurious methods
> "get_interface_details" and "get_all_attached_interfaces" so this branch
> should merge trunk changes.
>

This is something I wanted to ask you. Why these two methods are not in trunk? I thought they were there (and if I implemented against them I reckon they were in the plugin interface)

get_interface_details is the method using for retrieving the attachment for a port
GET /tenants/{tenant_id}/networks/{network_id}/ports/{port_id}/attachment
On the other hand, get_all_attached_interfaces is not used by any API operation so it could be removed.

We can do two things:
1) restore get_interface_details in the plugin interface
2) use get_port_details for retrieving the attachment.

The second option would lead to another question: do we need the attachment resource at all? Is the attachment a resource, or just an attribute of the port?

> 8) I would be ok with delaying the merge till unit tests are there would be
> good by me, if thats less work for you.

It's not about doing more or less work, it is about the quality of the code that goes in trunk.
It is my opinion that we should make a rule that only code for which enough test cases are provided should land in trunk.

Reverting to "work in progress" while the comments are being addressed.

> Hi Somik,
> thanks for your review!
>
> The only point which probably deserves a bit of discussion is #7. Please let
> me know what you think of my proposal.
>
> > 1) We shouldn't be checking in .project .pydevproject and
> .pydevproject.moved
> > files.
> I should have messed up the commit operation. I will remove them from source
> control.
>
> > 2) pep8.errors seems out of date, I merged Santhosh and Rajaram's pep8 fixes
> > branch and trunk is pep8 clean.
>
> If you are talking about the pep8.errors file this shouldn't be there just as
> .project and the other .* files. It is a file which I generate with a script
> before commit.
>
> > 3) I got the branch and there are 2 minor pep8 issues:
> >
> > ./tests/functional/test_service.py:52:1: W293 blank line contains whitespace
> > ./tests/functional/test_service.py:54:5: E303 too many blank lines (2)
> >
>
> The script I was talking about needs to be improved... I copied it from my
> nova development environment, where the tests directory is under nova. This
> means I did not checked pep8 errors in tests before commit.
>
> > 4) I would be ok with leaving the debug messages in, if the user doesn't
> want
> > the debug messages they could change the log level.
> >
>
> No, it is not ok to leave the debug messages, especially if they are prints
> and not LOG.debug. I thought I had removed all the unnecessary ones.
>

print should definitely be removed.

I was talking about LOG.debug statements, we can leave those in.

> > 5) Based on the API spec. The accepted values for port-sate are:
> > * ACTIVE
> > * DOWN
> > So, we should update that to be consistent, currently the code uses values
> > "UP" and "DOWN" at some places.
> >
>
> Rigth. Should we use an enumeration in place of the hardcoded strings?
>

That would be good - we could file a bug and tackle it separately.

> > 6) In openvswitch/README , the change from quantum-framework -> quantum-
> > should be "quantum" no dash at end.
> >
>
> I don't know how that file got changed. I will revert the changes in it, as I
> did not want to make any change :-)
>
> > 7) The trunk doesn't have the following spurious methods
> > "get_interface_details" and "get_all_attached_interfaces" so this branch
> > should merge trunk changes.
> >
>
> This is something I wanted to ask you. Why these two methods are not in trunk?
> I thought they were there (and if I implemented against them I reckon they
> were in the plugin interface)
>
> get_interface_details is the method using for retrieving the attachment for a
> port
> GET /tenants/{tenant_id}/networks/{network_id}/ports/{port_id}/attachment
> On the other hand, get_all_attached_interfaces is not used by any API
> operation so it could be removed.
>
> We can do two things:
> 1) restore get_interface_details in the plugin interface
> 2) use get_port_details for retrieving the attachment.
>
> The second option would lead to another question: do we need the attachment
> resource at all? Is the attachment a resource, or just an attribute of the
> port?
>

get_all_attached_interfaces is definitely superfluous.

You are correct. get_port_details and get_interface_details did the same thing ...

> Hi Somik,
> thanks for your review!
>
> The only point which probably deserves a bit of discussion is #7. Please let
> me know what you think of my proposal.
>
> > 1) We shouldn't be checking in .project .pydevproject and
> .pydevproject.moved
> > files.
> I should have messed up the commit operation. I will remove them from source
> control.
>
> > 2) pep8.errors seems out of date, I merged Santhosh and Rajaram's pep8 fixes
> > branch and trunk is pep8 clean.
>
> If you are talking about the pep8.errors file this shouldn't be there just as
> .project and the other .* files. It is a file which I generate with a script
> before commit.
>
> > 3) I got the branch and there are 2 minor pep8 issues:
> >
> > ./tests/functional/test_service.py:52:1: W293 blank line contains whitespace
> > ./tests/functional/test_service.py:54:5: E303 too many blank lines (2)
> >
>
> The script I was talking about needs to be improved... I copied it from my
> nova development environment, where the tests directory is under nova. This
> means I did not checked pep8 errors in tests before commit.
>
> > 4) I would be ok with leaving the debug messages in, if the user doesn't
> want
> > the debug messages they could change the log level.
> >
>
> No, it is not ok to leave the debug messages, especially if they are prints
> and not LOG.debug. I thought I had removed all the unnecessary ones.
>
> > 5) Based on the API spec. The accepted values for port-sate are:
> > * ACTIVE
> > * DOWN
> > So, we should update that to be consistent, currently the code uses values
> > "UP" and "DOWN" at some places.
> >
>
> Rigth. Should we use an enumeration in place of the hardcoded strings?
>
> > 6) In openvswitch/README , the change from quantum-framework -> quantum-
> > should be "quantum" no dash at end.
> >
>
> I don't know how that file got changed. I will revert the changes in it, as I
> did not want to make any change :-)
>
> > 7) The trunk doesn't have the following spurious methods
> > "get_interface_details" and "get_all_attached_interfaces" so this branch
> > should merge trunk changes.
> >
>
> This is something I wanted to ask you. Why these two methods are not in trunk?
> I thought they were there (and if I implemented against them I reckon they
> were in the plugin interface)
>
> get_interface_details is the method using for retrieving the attachment for a
> port
> GET /tenants/{tenant_id}/networks/{network_id}/ports/{port_id}/attachment
> On the other hand, get_all_attached_interfaces is not used by any API
> operation so it could be removed.
>
> We can do two things:
> 1) restore get_interface_details in the plugin interface
> 2) use get_port_details for retrieving the attachment.
>
> The second option would lead to another question: do we need the attachment
> resource at all? Is the attachment a resource, or just an attribute of the
> port?
>
>
> > 8) I would be ok with delaying the merge till unit tests are there would be
> > good by me, if thats less work for you.
>
> It's not about doing more or less work, it is about the quality of the code
> that goes in trunk.
> It is my opinion that we should make a rule that only code for which enough
>...

Hi Somik,
I have addressed your comments and updated the branch.

We (me and Brad) are now working on unit tests. I will therefore delay this merge proposal until the tests land in trunk, unless somebody raises the importance for the two bugs addressed by this branch.

I am reproposing this branch for merge.

This branch addresses some changes in the API requested by Erik Carlin; in particular it adds the /detail action on network objects.

Also it fixes the following bugs:

bug #798261 - Quantum API fails to generate JSON faults
bug #798262 - Faulty XML de-serialization

After merging this branch, only 7 of the current unit tests will fail. These failures are due to the following bugs:

bug #814517 - XML serializer adds extra spaces / newlines to text
bug #814518 - Request body parser should reject body with invalid elements

A fix for the above mentioned bugs will be provided in a separate branch.

NOTE: This branch does not provide re-alignment of API code with specification. Work on API alignment is being carried out in lp:~salvatore-orlando/quantum/quantum-api-alignment (see also bug #813433)

Looks good to me. This merge should get us pretty close to getting our unit tests in order.

Salvatore, Perhaps I'm missing something, but I'm a bit confused about why this adds a bin/quantum.py that seems to have the same content as bin/quantum . Generally speaking I'd prefer to not have the same logic in two places, as it makes updating things harder.

> Salvatore, Perhaps I'm missing something, but I'm a bit confused about why
> this adds a bin/quantum.py that seems to have the same content as bin/quantum
> . Generally speaking I'd prefer to not have the same logic in two places, as
> it makes updating things harder.

My fault.
This is the file I use for debugging quantum in eclipse. Should have not been in the branch.

I don't have a lot of context on this part of the code, but this looks good to me if the above file is removed.

Only other thing I noticed was that api/__init__.py adds an import for pprint, but I don't see it used anywhere in the diff. Not a big deal.

The extra file and the unused import have been removed.
Dan, thanks for spotting this.

Salvatore

Seems like your last commit did not actually remove the bin/quantum.py file, but I'll switch this to approve as that was your intent, then remove the file as part of the merge.

Thanks for fixing the branch Dan!
I have checked the unit tests. We now have just 4 tests failing: 2 for bug #814518, and 2 for a small glitch in the unit test itself, which I'm going to fix over the weekend.

Ah, you're still awake :)

Sorry, I pushed that branch assuming you were out for the night.

I can take a crack at the bugs as well. The one is just a matching issues
in the test itself, I have it fixed locally but am in a meeting right now
and haven't pushed.

On Fri, Jul 29, 2011 at 5:12 PM, Salvatore Orlando <
<email address hidden>> wrote:

> Thanks for fixing the branch Dan!
> I have checked the unit tests. We now have just 4 tests failing: 2 for bug
> #814518, and 2 for a small glitch in the unit test itself, which I'm going
> to fix over the weekend.
> --
>
> https://code.launchpad.net/~salvatore-orlando/quantum/quantum-api/+merge/65979
> You are reviewing the proposed merge of
> lp:~salvatore-orlando/quantum/quantum-api into lp:quantum.
>

--
~~~~~~~~~~~~~~~~~~~~~~~~~~~
Dan Wendlandt
Nicira Networks, Inc.
www.nicira.com | www.openvswitch.org
Sr. Product Manager
cell: 650-906-2650
~~~~~~~~~~~~~~~~~~~~~~~~~~~

Actually, the other issue seems to be more fundamental than I thought. It turns out that JSON and XML deserialization is inconsistent with respect to integers. JSON will correctly deserialize an integer into a python integer, but XML will not. If I change the test to expect an int or a string, one of the two tests will fail. Changing the actual PortCount value returned by the API server to a string works, but is not the long-term fix.