Conversation
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
|
CC @nhoening |
Documentation build overview
Show files changed (10 files in total): 📝 9 modified | ➕ 0 added | ➖ 1 deleted
|
|
Thank you for requesting a review. It looks like the scope of this PR may be mistaken? You are creating an endpoint to copy all assets under a given account*, rather than an endpoint to copy a given asset, including all assets under it. |
|
We can also discuss the test strategy. Joshua chose to implement a part of the solution, then tests for that, then the next part, test for that etc. I believe we suggested initially to have one test case that tests a complete successful copy, then work on making it pass.
|
Signed-off-by: Joshua Edward <oghenerobojosh01@gmail.com>
Signed-off-by: Joshua Edward <oghenerobojosh01@gmail.com>
Flix6x
left a comment
There was a problem hiding this comment.
Thanks @joshuaunity this is now heading in the right direction. See my comments for the next challenge: a deep copy. Just let me know if any decisions come up.
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Flix6x
left a comment
There was a problem hiding this comment.
A few comments on the schema. I'll move to testing now.
Flix6x
left a comment
There was a problem hiding this comment.
I was hoping to use Swagger for testing, but I don't see the new endpoint listed on http://localhost:5000/api/v3_0/docs/. I'm delaying testing until I can use that. If you need me to test without it, please provide instructions.
…s and query parameters Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
…ated Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Co-authored-by: Felix Claessen <30658763+Flix6x@users.noreply.github.com> Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
Fixed in commit |
There was a problem hiding this comment.
While testing, a lot works very well now. A few things left:
- Regression: on opening the Edit Graphs modal, I get toast notifications showing "Unknown graph format: 124". It doesn't happen on
main. Can you reproduce it? - I also had copilot fix the failing pipeline, but I still see the POST annotations endpoint being deleted from
openapi-specs.jsonin this PR. I don't know why, please investigate. - When copying an asset twice, the second copy gets the same name as the first copy. I think subsequent copies should get a number, so:
- Home
- Home (Copy)
- Home (Copy 2)
- Home (Copy 3)
- etc.
- Please update the PR description.
- Missing changelog entry.
What does your sensors_to_show look like on your DB |
…ndpoint for asset annotations Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
Are we now at 100% or are there still cases where you expect copying flex-config to fail? |
|
thanks |
Signed-off-by: JDev <45713692+joshuaunity@users.noreply.github.com>
we are at 100% i would say, as of my tests on Friday, flex config was copied over nicely. |
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
This is now fixed. |
There was a problem hiding this comment.
Two observations from testing:
- I found an interesting case that seems to hang the server: setting the parent ID to the asset ID being copied.
- The ID of a new asset (leaf node in an asset tree) seems to jump up by several thousand. I don't understand why. Isn't the next available integer chosen?
Are we now at 100% or are there still cases where you expect copying flex-config to fail?
we are at 100% i would say, as of my tests on Friday, flex config was copied over nicely.
Why don't you update the PR description?
I see, I forgot to add the "how to test". Sorry about that |
Nice find. The problem is a recursion bug. Here is the issue. When copying an asset, the function first creates and flushes the new copied asset under the requested target parent. After that, it queries for all children of the source asset to continue recursive copying. If the target parent is the same asset being copied (for example, copy asset 5 to parent 5), the just-created copy now appears in that child query. The recursion then treats this new copy as another source child to copy, creates another copy, and repeats indefinitely, causing a recursion loop until Python raises RecursionError. The first thing that pops into my head is to add validation to prevent this. What do you think? |
After multiple tests, I noticed it, but I can't pinpoint a part of the code that can do this, except for the "flush" part. The logic can inflate the sensor sequence, especially during recursive copies or failed recursion loops. Now, maybe it was caused by the recent bug you caught, and at 'flush time," when the endless loop was running, thousands of DIs were consumed. Use the below to reset your sensor and generic_asset IDs and test again SELECT setval('sensor_id_seq', , false); |
|
Thanks, I'll test with that. And adding validation to prevent the recursion sounds like a good plan. I did not yet test setting the parent to a child of the original asset, but I would imagine that would lead to the same kind of recursion. |
Ill also test that case |
…cendants Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
This has now been implemented |
Description
This PR introduces an API that allows users to copy assets from one account to another (or the same account), using the original asset and its children as templates.
##TODO
Look & Feel
None
How to test
(/assets/<asset_id>/copy). Put in your destination account (the account you want to copy to) and your parent asset (the asset you want to copy over to)—all as query params. See Swagger docs for more details on the API.Further Improvements
None
Related Items
This PR closes #1966
Sign-off