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

Added a new debug host field to AWS plugin global settings #1402

Closed
wants to merge 3 commits into from

Conversation

tbtmuse
Copy link

@tbtmuse tbtmuse commented Nov 28, 2019

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Description

Added a new text field above cloud debug called 'Local debug host', this value is saved on a global AWS plugin level and is then used by each debug extension in place of the hardcoded "localhost" host.

In the event that the local debug host field is left blank, the debug host defaults to "localhost".

Motivation and Context

The hard coded debugger host in the debug extensions support files, made it impossible to debug lamda functions locally when the debug host is not accessible via "localhost". This change exposes that setting allowing better flexibility.

Related Issue(s)

#642 #784

Testing

Tested changes by running local gradle tests and making sure those passed, and by building the plugin and running it in intellij ultimate using the debugging locally guide. Also testing both scenarios where no value is specified and where value is specified.

Testing environment:
Windows 10 - Slow ring build 19033
SAM CLI: 0.31.1
AWS CLI: 1.16.280
Pycharm & Intellij Ultimate

Screenshots

screenshot

Checklist

  • I have read the README document
  • I have read the CONTRIBUTING document
  • Local run of gradlew check succeeds
  • My code follows the code style of this project
  • I have added tests to cover my changes
  • All new and existing tests passed
  • A short description of the change has been added to the CHANGELOG

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-io
Copy link

codecov-io commented Nov 28, 2019

Codecov Report

Merging #1402 into master will decrease coverage by 14.52%.
The diff coverage is 34.28%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #1402       +/-   ##
=============================================
- Coverage     54.48%   39.95%   -14.53%     
+ Complexity     1183      922      -261     
=============================================
  Files           297      297               
  Lines         11255    11123      -132     
  Branches       1451     1418       -33     
=============================================
- Hits           6132     4444     -1688     
- Misses         4452     6168     +1716     
+ Partials        671      511      -160
Flag Coverage Δ Complexity Δ
#integtest ? ?
#uitest ? ?
#unittest 39.95% <34.28%> (+0.08%) 922 <3> (-5) ⬇️
Impacted Files Coverage Δ Complexity Δ
...ins/services/lambda/execution/local/SamDebugger.kt 0% <0%> (-52.64%) 0 <0> (-4)
...services/lambda/execution/local/SamDebugSupport.kt 20% <0%> (-30%) 0 <0> (ø)
...ns/services/lambda/python/PythonSamDebugSupport.kt 0% <0%> (-81.82%) 0 <0> (-3)
...brains/services/lambda/java/JavaSamDebugSupport.kt 7.69% <0%> (-27.61%) 0 <0> (-2)
...ns/services/lambda/dotnet/DotNetSamDebugSupport.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...ns/services/lambda/nodejs/NodeJsSamDebugSupport.kt 0% <0%> (-54.06%) 0 <0> (-4)
...ts/jetbrains/settings/AwsSettingsConfigurable.java 54.88% <54.54%> (-13.18%) 19 <1> (-9)
.../toolkits/jetbrains/settings/LocalDebugSettings.kt 54.54% <54.54%> (-12.13%) 2 <2> (ø)
...rvices/clouddebug/execution/steps/PreStartSteps.kt 0% <0%> (-100%) 0% <0%> (-3%)
.../software/aws/toolkits/jetbrains/core/HttpUtils.kt 0% <0%> (-100%) 0% <0%> (ø)
... and 107 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24cda55...1a850ad. Read the comment docs.

Copy link
Contributor

@kiiadi kiiadi left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

What's the motivation around having this as a global setting vs it being part of the Run Configuration itself where it'll be more discoverable? Is this something you see working for the ECS cloud debugging feature too?

Are there other IntelliJ-wide settings we could/should use instead? I imagine that remote debug of regular applications runs into the same problem right?

@tbtmuse
Copy link
Author

tbtmuse commented Nov 30, 2019

@kiiadi thanks for taking the time to review.

To my understanding the Sam executable setting is unlikely to change per run/debug configuration and so is the location of the cloud debug cli executable, and this is why they are global (I could be very wrong).

I also considered the debugger host to be a setting that is unlikely to change per run/debug configuration, this is why I made it global.

I don't see this working with the ECS cloud debugger feature as they both seem very specific to the type of code, lambda VS code sitting in an Amazon ECS with AWS Fargate.

I do not think there are other intellij wide settings that can be used instead.

Not sure about the remote debugging of regular applications and this problem, but I think the issue would be the same.

@tbtmuse tbtmuse closed this Nov 30, 2019
@tbtmuse tbtmuse reopened this Nov 30, 2019
@tbtmuse tbtmuse requested a review from kiiadi December 2, 2019 09:25
@kiiadi
Copy link
Contributor

kiiadi commented Dec 13, 2019

Just circling back here - I want to understand a bit more about your setup to see why we need this? Is there a way we can 'infer' the "local" connection? e.g would replacing hard-coded localhost with InetAddress.getLocalHost() and/or InetAddress.getLoopbackAddress() work instead?

@tbtmuse
Copy link
Author

tbtmuse commented Dec 21, 2019

The only way I can see this working without allowing a user to explicitly set the IP of their debug host, is to determine if Intellij is connected to Docker via Docker Machine and then grabbing the IP of the Docker host somehow.

InetAddress.getLocalHost() and/or InetAddress.getLoopbackAddress() will not be good enough.

@abrooksv
Copy link
Contributor

Are you able to connect to Docker from the IDE?

@tbtmuse
Copy link
Author

tbtmuse commented Dec 30, 2019

Yes indeed, I am able to connect from the IDE.

@abrooksv
Copy link
Contributor

abrooksv commented Dec 30, 2019

Querying the Docker plugin isnt too difficult, the com.intellij.docker.DockerCloudConfiguration contains the API URL, so we should be able to get that and then say .host() on the URI if the URI is not a unix socket.

Does your Docker Config API URL contain the same IP you need to debug?

@tbtmuse
Copy link
Author

tbtmuse commented Dec 30, 2019

Yes it does contain the same IP I need to debug.

@abrooksv
Copy link
Contributor

abrooksv commented Jan 3, 2020

#1484 stores it in the run config and queries it from Docker plugin

@kiiadi kiiadi mentioned this pull request Jan 6, 2020
9 tasks
@abrooksv
Copy link
Contributor

abrooksv commented Jan 7, 2020

@tbtmuse Thank you for the pull request and bringing this to our attention. I have merged #1484 that extended your idea but bases this on your docker settings, you can select the host on the SAM CLI page of the Run Configuration.

@abrooksv abrooksv closed this Jan 7, 2020
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.

4 participants