refactor input functions to make them testable

[taimoorgit]

Issues

https://github.com/OpsLevel/team-platform/issues/264

Changelog

• Divide readResourceInput into 3 functions: one to read input, one to apply input, one to do both
• Change input function signatures to allow injecting test data by using variadic arguments
• Add tests for input functions

[rocktavious]

It looks like this is ONLY used in test files - this shouldn't be part of the codebase then it should be a utility function in the testing code.

[rocktavious]

Same with its helper functions handleJSONString and handleJSONSchema

[taimoorgit]

@rocktavious this is used in the properties PR that follows this: #257

[rocktavious]

In fact after reading the whole code - i don't see where this function is used - so why are we testing it?

[taimoorgit]

@rocktavious I wanted to add an abstraction in case we would start adding more resource types that use JSON fields outside of properties, but I have now reversed this.

[rocktavious]

I feel like the abstraction is more complex and unexpected then having custom yaml marshallars

[taimoorgit]

@rocktavious if we remove support for JSON, we could replace this code with only custom YAML marshalers which would be much cleaner. Supporting both YAML and JSON feels hacky.

Also, while you can read in JSON objects, reading in JSON values (number, string, boolean, list) is next to impossible without us writing another 100+ lines of code.

How do you feel about only supporting YAML?

[rocktavious]

#255 (comment)

I think i already voiced my approval of having the yaml way only

[rocktavious]

The only reason input is here is because of the tests - we should refactor this so it doesn't take input (because the only thing i should be doing is "reading input"

The function that takes an input argument should be a helper function in the test code.

this will also remove the need to add all the (nil) to the codebased where this function is used.

In the future it's worth remembering to not conflate test code with production code. Keeping separated concerns and making a function do 1 logical thing will help.

[taimoorgit]

@rocktavious being able to pass in input can serve a purpose outside of tests as well, for example by being to do readInput() to get input in bytes that can be used to get the raw input from the user before calling ReadResourceInput().

[rocktavious]

This add complexity for a "what if" that we are currently not doing in our code outside of testing nor would likely do IMO.

Is there any reason that the tests cannot just use ReadResource[T](input) ?

[taimoorgit]

@rocktavious I have changed the ReadResourceInput signature to use a variadic argument for the mocked inputs. Is that OK? It means that we don't need to (nil) every time.

[rocktavious]

thats definitely better - i still don't like it because now you are abusing variadic - especially because you don't handle multiple arguments so if someone passes 2 things they get unexpected behaviour of reading from standard input or from a file - but we can move on since this gets what i'm after it just leaves tech debt in our codebase sadly.