main: add regression test for aws upload target arch error

[mvo5]

This is a regression test to ensure the commandline handling
of aws upload is correct. Its a followup for
#1030

[lzap]

I would not do this, a global variable is sub-par even for testing. The function called handleAWSFlags should have an argument that takes an interface, an uploader. The real code would pass a real implementation while the test would pass a mock.

Also, it smells like function called "handle flags" is perhaps doing too much and should be broken into smaller pieces, or maybe renamed to "initialize AWS".

[mvo5]

I would not do this, a global variable is sub-par even for testing. The function called handleAWSFlags should have an argument that takes an interface, an uploader. The real code would pass a real implementation while the test would pass a mock.
[..]

Saying "it's sub-par even for testing" is a strong statement. Sometimes the dependency injection approach, sometimes the monkey patching approach is more appropriate. This style of having a var to mock is something I have seen in many go projects. Here is an example where we used it to make things easier (vs dependency injection) https://github.com/osbuild/bootc-image-builder/pull/1018/files - i.e. monkey patching can often avoid making the API more involved just so that its testable on the expense of an (internal) var like here.

[lzap]

I am sorry but I see this and the linked patch as inferior solution to the problem. (Small) interfaces are cornerstone of Go, I see no reason why extractTLSKeys would accept anything else than io.Reader. Surely mocks do have its place but messing around with global state like that to test one function is an overkill in my eyes. I cannot imagine all functions tested like that.

My acceptance threshold for monkey-patching is likely very different from yours. Granted I spent 10 years working on a massive Ruby on Rails project where this was bread and butter and learned to I hate it. Anyway, this isn't a review, I was just randomly commenting. Please carry on.

[mvo5]

I am sorry but I see this and the linked patch as inferior solution to the problem. (Small) interfaces are cornerstone of Go, I see no reason why extractTLSKeys would accept anything else than io.Reader. Surely mocks do have its place but messing around with global state like that to test one function is an overkill in my eyes. I cannot imagine all functions tested like that.

My acceptance threshold for monkey-patching is likely very different from yours. Granted I spent 10 years working on a massive Ruby on Rails project where this was bread and butter and learned to I hate it. Anyway, this isn't a review, I was just randomly commenting. Please carry on.

While I agree that testing every function with monkey patching is silly (and dependency injection is super useful) I guess we have to agree to disagree. If used with discipline and in a predictable pattern (i.e. not random messing with global state but very controlled and predictable) I think monkey patching is a fine tool and often avoids extra complications that would otherwise be needed.

[lzap]

I think we should break the main package into multiple files or even packages after this is merged into ibcli.

[lzap]

Intentional empty line?

[mvo5]

No, silly me. I will fix it (thanks!)

[lzap]

Needs a rebase.