Add run local in debug

[mathianasj]

Description of the change:
Add a debug flag for operator-sdk up local, which wraps the built binary into the dlv command to attach a remote debugger

Motivation for the change:
To simplify the process to attach a debugger when developing an operator locally

[openshift-ci-robot]

Hi @mathianasj. Thanks for your PR.

I'm waiting for a operator-framework or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[mathianasj]

Looks like there are some unit tests I need to resolve. I will work on those and update.

[AlexNPavel]

/ok-to-test

[hasbro17]

Also this flag needs a mention in the Added section of the CHANGELOG.

[mathianasj]

Thanks, I will try to get those updated today or tomorrow.

[mathianasj]

@hasbro17 I completed adding the args to the debug command, updated the changelog, and tests are now passing. Let me know if you see anything else.

[AlexNPavel]

@mathianasj Overall looks good. I am unsure how useful delve is if we are using a standard compiled go binary due to optimizations. Delve's docs say that you should add -gcflags="all=-N -l" to the go build command to ensure that you have proper debugging symbols: https://github.com/go-delve/delve/blob/master/Documentation/usage/dlv_exec.md#synopsis

This should be pretty easy to add since we have a buildLocal function in this file and we can just check if the debug flag is set to apply the args. Can you add that change to this PR?

[mathianasj]

@AlexNPavel I updated to include the debug symbols if debug flag is specified

[AlexNPavel]

LGTM

[joelanford]

I think this is well implemented and looks pretty good, with just one comment about the flag/variable name. 👍

Having said that, I'm a little uneasy about these kinds of feature additions into the SDK. This could just as easily be implemented by a user in their own Makefile.

We've recently added support for customizing CGO_ENABLED and go's -ldflags options, and it seems like the flag set could balloon as more edge cases need to be handled.

I wonder if we (and ultimately users) would be better off if we kept the SDK simple for the straightforward use cases and and documented ways for users with complex or uncommon use cases to customize their builds with Makefiles?

Kubebuilder defers basically all non-scaffolding tasks to a generated Makefile, giving users all the flexibility (and responsibility) to tune and change as they see fit. I'm not saying we should swing that far the other direction, but I think we should discuss where the line should be drawn.

Thoughts @hasbro17 @AlexNPavel @shawn-hurley @estroz @lilic @theishshah?

[mathianasj]

@joelanford updated flag and associated documentation

[estroz]

@joelanford I agree. There have been quite a few additions to commands over time that we'll have to maintain as long as they're in the project. Adding more non-global or niche-use-case flags is unsustainable. We should have a discussion about how to add these features to commands while keeping the CLI slim.

[mathianasj]

Any updates on this, I noticed at least one other was looking for functionality such as this

[syedriko]

IMO, operator-sdk needs this feature as long as it wraps building, testing and running of operators. I needed this as soon as I started debugging my first operator and wrote something similar: https://github.com/syedriko/operator-sdk/tree/up_local_dlv before @AlexNPavel pointed me to this PR.

I too agree that going forward this wrapping approach can quickly become unsustainable. May be the way to go is to deprecate the build, olm-catalog, print-deps, run, scrorecard, test and up commands, implement them as make targets and until they are removed from the operator-sdk executable entirely, implement them as invocations of make <target>?

[AlexNPavel]

@joelanford In the future, we should modify this to allow more customization from the users, but I think we should add this functionality to the SDK for now as there are multiple users asking for this functionality, and we can discuss ways to have a more customizable design later.

[joelanford]

@AlexNPavel It does seem like there's growing interest in this, so I'm okay with merging this feature and sorting out how to handle these kinds of things later.

@mathianasj Can you also add a line to the README's Prerequisites section? Something like:

* Optional: [`delve`](https://github.com/go-delve/delve/tree/master/Documentation/installation) version 1.2.0+ (for `up local --enable-delve`).

LGTM once these are addressed.

[mathianasj]

@joelanford updated changes you requested, I also do agree that the sdk should be limited to developer resources, and that the cli in the way it currently is designed requires adding functionality such as this. I would be happy to be involved in discussions for future development and changes. I know that the customer I am currently working with has numerous planned operators and I could get their input also.

[joelanford]

LGTM

[hasbro17]

LGTM after nit.

[mathianasj]

Updated

[hasbro17]

@mathianasj Thanks for adding this in.

[syedriko]

Yay! Thanks for merging this, guys!