Fix how duplicates are handled in ONVIF [SAME VERSION] #393
Conversation
kate-goldenring
requested review from
bfjelds,
Britel,
jiria and
romoh
as code owners
this seems fine ... i'm not super worried about versions 6.17 and 6.18 not working. bugs happen. |
| @@ -341,7 +345,7 @@ pub mod util { | |||
| "simple_onvif_discover - uris discovered by udp broadcast {:?}", | |||
| broadcast_responses | |||
| ); | |||
| let mut filtered_uris = Vec::new(); | |||
| let mut filtered_uris = std::collections::HashSet::new(); | |||
| broadcast_responses.into_iter().for_each(|r| { | |||
| filtered_uris.extend(get_scope_filtered_uris_from_discovery_response( | |||
There was a problem hiding this comment.
filtered_uris are created by get_scope_filtered_uris_from_discovery_response which gets the uris from each discovery response. execute_filter is done when looking at the body of the discovery response. It is a text based (not api call) filter. It doesnt seem to make sense to get the uris and then do an api call to get scopes and filter
|
Is there a way to |
I think that would definitely be the approach if other commits (changing versions) were added between the release and discovering the bug. Fortunately, since it was discovered quickly, we can effectively overwrite the release Not to mention, i already deleted the release. Its that the containers and helm package still exist |
| @@ -310,7 +314,7 @@ pub mod util { | |||
| scopes_filters: Option<&FilterList>, | |||
| timeout: Duration, | |||
| ) -> Result<Vec<String>, anyhow::Error> { | |||
| let mut broadcast_responses = std::collections::HashSet::new(); | |||
| let mut broadcast_responses = Vec::new(); | |||
There was a problem hiding this comment.
That can be a HashSet too, but I guess it doesn't provide much value at this scope.
Sounds good. yanked releases happen. Glad you found it early on. |
Yeah i wonder if we want to try to set up e2e testing for onvif maybe that only runs on release or can be triggered by a |
Signed-off-by: vincepnguyen <[email protected]>
What this PR does / why we need it:
There is an ONVIF discovery bug caused by this PR #382, which means that ONVIF discovery doesn't work for Akri versions 0.6.17-0.6.19. Any thoughts on how this should be handled? This is further compounded by the fact that I discovered this after creating the release, so currently latest points to 0.6.19 which means
helm install akri akri-helm-charts/akriinstalls the faulty version.My proposed fix: push this fix as version 0.6.19 and create new release and in the release CHANGELOG (see #394) put that ONVIF discovery does not work in versions 0.6.17 and 0.6.18 .
Thoughts?
Special notes for your reviewer:
The bug was caused by the fact that we were applying
to_lowercase()on xml strings to be passed to later parts of discovery and leaving cameras un-discovered. To fix this, this PR removes duplicates on only the URIs returned fromget_scope_filtered_uris_from_discovery_responseis more ideal.Example of issue: original string:
And after
to_lowercase():If applicable:
cargo fmt)cargo build)cargo clippy)cargo test)cargo doc)./version.sh)