hybrik: fix bug with improper connections

[zsiec]

This is a small patch to address a bug that was causing incorrect connections to be constructed. Connections were generated referencing non-existent elements and some elements were left unconnected.

[codecov-io]

Codecov Report

Merging #211 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #211   +/-   ##
=======================================
  Coverage   78.62%   78.62%           
=======================================
  Files          29       29           
  Lines        2914     2914           
=======================================
  Hits         2291     2291           
  Misses        372      372           
  Partials      251      251

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bfc86b...16eae21. Read the comment docs.

[fsouza]

Hi @zsiec, thank you very much for contributing! :D Overall looks good, but maybe we can avoid some repetition.

I had forgotten that we never added tests to the hybrik provider. Perhaps we can take a look at doing that next? x)

[fsouza]

typo:

Suggested change

	transcodeElementIDTempate = "transcode_task_%s"
	transcodeElementIDTemplate = "transcode_task_%s"

[zsiec]

👍 thanks for catching

[fsouza]

Could we range through elements and add element.UID to the transcodeSuccessConnections slice? Something like:

for i, e := range elements {
  transcodeSuccessConnections[i] = hwrapper.ToSuccess{Element: e.UID}
}

Then we don't need an additional slice, I think. And that would also keep the logic to generate the element id inside the monutTranscodeElement function.

If we do that, can you also make the constant transcodeElementIDTemplate local to that function?

[zsiec]

This change was made because elements holds a slice of all elements and in this case, not only the transcode task elements, but at least one source element created and appended before we run the logic to create the transcode task elements. (this was causing incorrect connections to be generated)

Rather than assume we have a single source element and doing a range over elements[1:] (along with assuming there are no elements between the source and the transcode tasks), I thought it would be less error prone to explicitly track transcode tasks. This allows us to freely add elements before the transcode elements without having to change that slice offset.

We can safely change
transcodeElementIds = append(transcodeElementIds, fmt.Sprintf(transcodeElementIDTemplate, taskIndex))
to
transcodeElementIds = append(transcodeElementIds, e.UID)

but we make use of transcodeElementIDTemplate again in presetsToTranscodeJob when generating connections to packagings tasks (line 293) (perhaps we could send the transcodeElementIDTemplate into mountTranscodeElement to keep it out of the global scope?)

[fsouza]

Thanks for clarifying! This makes sense, my bad x)

We can keep the code as is :D

[zsiec]

Cheers! I was tripped up by the code a few times myself, I think we can clean it up a little :)

[zsiec]

@fsouza thanks for the review! This was a simple patch to get things working again, We will likely take a pass at rewriting it completely, similar to what we did for the new Bitmovin provider in #210. (although abstractions will diverge slightly as the Hybrik and Bitmovin APIs differ) We want to simplify the functions and reduce the amount of state that shares scope, and of course, add tests :)

[scentless-apprentice]

🚀