Skip to content

Add --debug build flag to include Delve debugger#1148

Closed
creydr wants to merge 3 commits intoko-build:mainfrom
creydr:add-debug-flag-to-include-delve
Closed

Add --debug build flag to include Delve debugger#1148
creydr wants to merge 3 commits intoko-build:mainfrom
creydr:add-debug-flag-to-include-delve

Conversation

@creydr
Copy link
Contributor

@creydr creydr commented Sep 13, 2023 •
edited
Loading

Currently the entrypoint of the image is always set to /ko-app/<app-name>. This makes it hard to remote debug the app via Delve, which invokes the app e.g. via dlv exec <path-to-app>).

This PR adds a new build flag (--debug), which adds the Delve debugger to the image and sets the entrypoint to invoke delve instead of the ko-app directly.

Hint:

How to test this PR:

  1. Apply a manifests with the debug flag. e.g. ko apply --debug -f config/controller.yaml
  2. Start a port forwarding for Delve: kubectl port-forward deploy/my-controller 40000:40000
  3. Connect from your IDE to the remote debugger. E.g. use the following VSCode launch config: 
    {
    "version": "0.2.0",
    "configurations": [{
            "name": "Attach to remote",
            "type": "go",
            "request": "attach",
            "mode": "remote",
            "port": 40000,
            "host": "0.0.0.0",
            "showLog": true,
            "cwd": "${workspaceFolder}"
        }
    ]
}

@creydr
Copy link
Contributor Author

creydr commented Sep 13, 2023

Hi @imjasonh,
regarding the discussion in #1138, I came up now with the --debug option as an alternative (and from a user perspective simpler) approach.

@creydr
Copy link
Contributor Author

creydr commented Sep 18, 2023

Hi @imjasonh,
could you check on this PR?

@cpanato cpanato requested a review from imjasonh September 18, 2023 15:24
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023 •
edited
Loading

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (cf3e626) 49.34% compared to head (15f5078) 49.87%.
Report is 20 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1148      +/-   ##
==========================================
+ Coverage   49.34%   49.87%   +0.53%     
==========================================
  Files          44       44              
  Lines        3640     3743     +103     
==========================================
+ Hits         1796     1867      +71     
- Misses       1614     1637      +23     
- Partials      230      239       +9
Files Coverage Δ
pkg/build/options.go 66.66% <100.00%> (+1.96%) ⬆️
pkg/commands/options/build.go 68.80% <100.00%> (+0.50%) ⬆️
pkg/commands/resolver.go 36.01% <0.00%> (-0.44%) ⬇️
pkg/build/gobuild.go 58.48% <65.95%> (+1.17%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@creydr
Copy link
Contributor Author

creydr commented Sep 21, 2023

Hello @jonjohnsonjr,
I saw your name in the MAINTAINERS.md file :) Could you check on this PR or help me to find someone who can review it?
Thanks a lot in advance

@creydr creydr mentioned this pull request Oct 3, 2023
Closed
@creydr
Copy link
Contributor Author

creydr commented Oct 10, 2023

Friendly reminder about this 😁

cpanato
cpanato reviewed Oct 10, 2023
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, but I am not sure if we want to install this tool without any checks,

but i will defer to @imjasonh

@creydr creydr force-pushed the add-debug-flag-to-include-delve branch from 15f5078 to 4007bd7 Compare November 8, 2023 16:30
@creydr
Copy link
Contributor Author

creydr commented Nov 8, 2023

Rebased to solve merge conflicts.
@imjasonh is there a way you find some time to check on this? Or any other advice how we can proceed here? If you don't want such functionality in ko, I have to accept it but it would be good for me to know if we can follow up here or if I have to find another way.
Thanks in advance already

@github-actions
Copy link

github-actions bot commented Feb 7, 2024

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@creydr creydr force-pushed the add-debug-flag-to-include-delve branch from 4007bd7 to 002f3cf Compare February 7, 2024 06:57
@github-actions
Copy link

github-actions bot commented May 9, 2024

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@creydr creydr force-pushed the add-debug-flag-to-include-delve branch from 002f3cf to 738c3b5 Compare May 13, 2024 14:34
creydr added 3 commits May 13, 2024 16:36
@creydr
Add debug option to include Delve debugger
4258225 
Signed-off-by: Christoph Stäbler <cstabler@redhat.com>
@creydr
Run ./hack/update-codegen.sh
11079e6 
Signed-off-by: Christoph Stäbler <cstabler@redhat.com>
@creydr
Fix typo
6778b74 
Signed-off-by: Christoph Stäbler <cstabler@redhat.com>
@creydr creydr force-pushed the add-debug-flag-to-include-delve branch from 738c3b5 to 6778b74 Compare May 13, 2024 14:37
@luhring luhring mentioned this pull request May 21, 2024
Merged
@luhring
Copy link
Contributor

luhring commented May 21, 2024

This looks so cool!!!!! I see that conflicts have emerged over time, and I opened #1320 to see if we can get this across the finish line 🏁

@creydr
Copy link
Contributor Author

creydr commented May 21, 2024

This looks so cool!!!!! I see that conflicts have emerged over time, and I opened #1320 to see if we can get this across the finish line 🏁

Hey @luhring,
thanks for checking on this. I rebased it a few days ago, but didn't see any merge conflicts on this (now they probably came via 2a4c12f). Thanks for addressing them already in #1320

@imjasonh
Copy link
Member

imjasonh commented Jun 11, 2024

This is merged! 🎉

Thank you for your patience, and happy debugging! 🐛

@imjasonh imjasonh closed this Jun 11, 2024
@creydr creydr mentioned this pull request Aug 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cpanato cpanato cpanato left review comments

@imjasonh imjasonh Awaiting requested review from imjasonh

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@creydr @codecov-commenter @luhring @imjasonh @cpanato