Skip to content
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

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

Closed
wants to merge 3 commits into from

Conversation

creydr
Copy link
Contributor

@creydr creydr commented Sep 13, 2023

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?

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023

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
Copy link
Contributor Author

creydr commented Oct 10, 2023

Friendly reminder about this 😁

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

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.

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.

Signed-off-by: Christoph Stäbler <cstabler@redhat.com>
Signed-off-by: Christoph Stäbler <cstabler@redhat.com>
Signed-off-by: Christoph Stäbler <cstabler@redhat.com>
@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

This is merged! 🎉

Thank you for your patience, and happy debugging! 🐛

@imjasonh imjasonh closed this Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants