-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: golang / dlv 1.23 support #139
base: duct-tape
Are you sure you want to change the base?
feat: golang / dlv 1.23 support #139
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
CLA signed |
Note previous (unmerged) PR #138 for 1.22 |
Would really be nice to get this merged. Odd how this repo seems so neglected. |
@syntastical I reached out in multiple places in Kubernetes slack and haven't gotten a response, just pinged again so we'll see 🤷 If anyone has connections to help get this through would be appreciated. |
@zarenner maybe try adding @briandealwis as a reviewer or maybe @GoogleCloudPlatform. The last merged pull request was approved by @briandealwis (#132). |
No permission to directly add reviewers unfortunately. I see some discussion in GoogleContainerTools/skaffold#9553 about org structure change. @ericzzzzzzz mentions to use @GoogleContainerTools/skaffold-team if I understand correctly, but that doesn't seem to resolve. Also adding @plumpy as recent approver in Skaffold repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, sorry for the unreasonable delay!
COPY delve-*.patch . | ||
RUN patch -p0 -d delve-source < delve-as-options.patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why you decided to just delete this rather than update it to apply to Delve 1.23.0. Are the issues Brian mentioned in this PR no longer relevant? From reading that PR, it sounds to me like this will make the image:
- not work with binaries compiled with Go >= 1.24, even though it may turn out to be fully compatible. (I'm guessing this is why you had to delete all the tests for the older versions?)
- not work at all because the connecting user is different. (Perhaps the tests in this repo don't really cover this issue? Or maybe there's some other reason it's no longer relevant?)
Obviously I'm somewhat less concerned about the first bullet, but the second one seems worrying. I'm not super familiar with all this, so maybe I'm overlooking something...
Delve 1.20 no longer works properly with newer Golang versions (e.g. err variables show as nil even when non-nil). Updating to 1.23.0 which appears to fix the issue.
I included some changes I needed to run the tests locally (e.g. disable pushing integration test images), not sure if needed for CI but I don't think should hurt. I also removed the tests for now unsupported Golang versions.