search

grafana / iot-sitewise-datasource

IoT Sitewise
Apache License 2.0
18 stars 9 forks source link

fix: use non-batch APIs at the edge #281

Closed tracy-french closed 7 months ago

tracy-french commented 8 months ago

What this PR does / why we need it: IoT SiteWise BatchGet* APIs are not available at the edge. This change utilizes the non-batch variants of the APIs when at the edge.

Which issue(s) this PR fixes: https://github.com/grafana/iot-sitewise-datasource/issues/161

tracy-french commented 8 months ago

@njvrzm Thank you for the early review!

I noticed most of your feedback is on the *_batch.go files, which is not new code. Those files are simply renamed to distinguish from the non-batch API variants. Do we need to address the issues you identified for the existing code in this PR?

njvrzm commented 8 months ago

Indeed, my apologies, it was only towards the end of reviewing that I saw that most of the code is just moved 😅 I generally try to "leave it better" but that doesn't really apply here, and no, you don't need to address most of my feedback in this PR. I'll resolve some of my comments and maybe come back to them later myself.

tracy-french commented 8 months ago

No worries! Thank you for clarifying!

idastambuk commented 8 months ago

Hi @tracy-french, I think it makes sense to rename the files, but it could create problems down the line when we lose their entire source control history. Would it be possible for you to redo this PR in a way that preserves those files' history by renaming them with git mv?

tracy-french commented 8 months ago

@idastambuk For sure! I'll make that change.

tracy-french commented 8 months ago

@idastambuk I renamed the files with git mv, yet it didn't appear to make any difference from what I had before. Is it supposed to preserve the blame? Reading through https://stackoverflow.com/questions/1094269/whats-the-purpose-of-git-mv makes it seem like it's just a convenience function and git doesn't handle renames using it differently than a renames done outside of git?

njvrzm commented 8 months ago

Hey @tracy-french, thanks for addressing the review feedback so far. ~I need to figure out how to trigger the drone build so we can make sure that check runs~ nm, figured it out and approved the build. Meanwhile I ran the tests locally and the tests in TestPropertyValueAggregate are failing - could you fix that please?

idastambuk commented 8 months ago

@idastambuk I renamed the files with git mv, yet it didn't appear to make any difference from what I had before. Is it supposed to preserve the blame? Reading through https://stackoverflow.com/questions/1094269/whats-the-purpose-of-git-mv makes it seem like it's just a convenience function and git doesn't handle renames using it differently than a renames done outside of git?

Hi @tracy-french, yes, I noticed the same, I think it might be because the original files were renamed, but they were replaced with files with the same name (property_values_non_batch.go became the original property_values.go). I overlooked that when I suggested the solution, sorry about that!

tracy-french commented 7 months ago

@idastambuk @njvrzm Feedback addressed! Thank you!

idastambuk commented 7 months ago

Hi @tracy-french it looks like backend tests are failing, can you take a look? If you need help running them locally, let us know!

tracy-french commented 7 months ago

@idastambuk Sorry about that! It looks like a forgot a file when creating the commit. Working on resolving now!

tracy-french commented 7 months ago

@idastambuk @njvrzm Could we please merge this change in? Thank you! :)