-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use connection request v2 spec #297
Conversation
The It is not quite clear to me if we should maintain backward compatibility with the original connection request format. If we want to maintain backward compatibility at least for a while, we will probably have to add a new endpoint to handle connection requests in v2 format? |
@sajith adding a new /connection/v2 endpoint is probably a good idea, so no need to overhaul all the unittests for the time being? in the swagger.yaml in sdx_lc, I've added the new request connection scheme there. |
@YufengXin A simpler solution might be to use oneOf, like so: /connection:
post:
operationId: place_connection
requestBody:
content:
application/json:
schema:
oneOf:
- $ref: '#/components/schemas/connection'
- $ref: '#/components/schemas/connection_v2'
required: true Now I need to figure out how to write a |
See these commits on sdx-lc: - 128c3445010310a3159c6c1b69e789ac6524d572 - f5a5f576b32d930a34628266d944fb7943057462 - 51f3a5349db2a7d3ed41d54f8d5466116b4bea10 - ab2f6f7df7e591735afd4c388c5a6de238f96ed4 - 22622d1cebb96f263fd8d41a9d27ba9a3479cfbe - 9c5963a9b9985a95b771b5f3cbeab9053bec8d76 (On branch 152-update-the-schema-on-connection-request-spec, PR: atlanticwave-sdx/sdx-lc#156)
When we use `oneOf` in request body schemas, the error message has less detail than it previously had.
Seems that we get two items in the response now, after adding another test with a different ID. Probably should investigate this further later.
Pull Request Test Coverage Report for Build 9783166269Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9785415149Details
💛 - Coveralls |
@YufengXin @congwang09 I'm keeping this in draft state since this uses code that is not in the The example connection request does not find a path. However we do generate a connection ID when it is not present (as in the case of v2 connection requests), and send a response like below, and a non-200 response code: {
"connection_id": "7e84c036-cc65-4257-9049-9b70992e8057",
"reason": "Could not generate a traffic matrix",
"status": "Failure"
} |
The modification looks good to me -:) For the unittest, in the draft PR in PCE,the test I added is this one: that uses this request connection in v2 format: tests/data/test_request-amlight_zaoxi-p2p-v2.json which finds the path on the three-topology |
Pull Request Test Coverage Report for Build 9788498633Details
💛 - Coveralls |
It would be useful to move However, for the purpose of the issue (namely, generating a connection ID), I think the newly added test should be sufficient. |
Pull Request Test Coverage Report for Build 9811578134Details
💛 - Coveralls |
Ok, added a test that finds a solution. Note that |
Cool! Now that the two tests worked, would you mind/have time to merge the PRs to main properly across the 4 repos with the right pointers in project files? -:) Otherwise I'll do it early next Monday. Cong would work on the test and I'll fix next week if these merges break anything |
Well, as long as you don't mind dealing with the fallout... Let me see how much I can get done today. :-) |
Pull Request Test Coverage Report for Build 9813294445Details
💛 - Coveralls |
@YufengXin @congwang09 I went ahead and merged atlanticwave-sdx/datamodel#131 and atlanticwave-sdx/pce#195. Leaving atlanticwave-sdx/sdx-lc#156 alone for now because I have some comments there. Ok, now you get to deal with the fallout from the above merges, while I get to do the main thing I've got to do. For a while anyway. ;-) |
This should probably wait for @congwang09. :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, and this is working fine.
Resolves #296. Work in progress, not quite there yet, etc. Updates in comments below.