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

[jb] add inspections & quickfixes on .gitpod.yml vmoptions config #11089

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yaohui-wyh
Copy link
Contributor

@yaohui-wyh yaohui-wyh commented Jul 1, 2022

Description

Implements Xmx config checks in JetBrains gitpod-remote plugin:

  • Add inspection of incorrect Xmx config
    • inspection
  • Add quickfix to add vmoptions settings if not exists
    • quickfix-add.mov
  • Add quickfix to correct vmoptions settings if runtimeXmx != configuredXmx or runtimeXmx != defaultXmx
    • quickfix-replace.mov

Close #11002

Related Issue(s)

Related #10924

How to test

Test locally (Run runIde task from IntelliJ plugin):

Release Notes

- Detect user changes to the max heap size of IDE server when using JetBrains Gateway, and suggest a user to put in .gitpod.yml to persist.

Documentation

Werft options:

  • /werft with-preview

@akosyakov
Copy link
Member

@yaohui-wyh It looks very promising!

Couple question:

  • Does it happen only if a user has .gitpod.yml opened? Can we run this inspection automatically on startup that a warning is visible in Problems even if .gitpod.yml is missing.
  • I'm not sure about the warning at the top. We are going to add more inspections, so maybe one warning like There are issues with your configuration which takes a user to Problems view to see all issues is enough?
  • Could we also add a quick fix to apply default or configured Xmx to the IDE?

@yaohui-wyh
Copy link
Contributor Author

yaohui-wyh commented Jul 4, 2022

1. Does it happen only if a user has .gitpod.yml opened? Can we run this inspection automatically on startup that a warning is visible in Problems even if .gitpod.yml is missing.

I'm afraid no automated project-wide inspections on IDE project startup is available:

TL;DR: Currently, the project-wide analysis works only in Java projects (and the analysis is disabled by default), users have to run inspection manually.

2. I'm not sure about the warning at the top. We are going to add more inspections, so maybe one warning like There are issues with your configuration which takes a user to Problems view to see all issues is enough?

Yeah, I did hesitate about the file-level warning. The tricky part is the Inspection warning (problemHolder) should attach to an AST Node (PsiElement) for IDE Editor to display UI hints (e.g. highlighted code block, color stripe in the scrollbar, etc.). When there's no "jetbrains" YAML key, the warning is registered to .gitpod.yml file, resulting in a warning at the top. I didn't find better candidates (AST Node) for attaching the warning to:
image

One possible way is to add a blank line to .gitpod.yml and attach the warning to the created blank line (an EOL is a valid PsiElement), however I don't think silently modifying the user's .gitpod.yml is better than a file-level warning. What's your advice?

3. Could we also add a quick fix to apply default or configured Xmx to the IDE?

I did add a quickfix for applying VMOptions in the CL ApplyVMOptionsQuickFix.kt, but suppress the usage of it in the following commit, I left a comment on #11002 (comment) for the reason. 💭

@akosyakov
Copy link
Member

Yeah, I did hesitate about the file-level warning. The tricky part is the Inspection warning (problemHolder) should attach to an AST Node (PsiElement) for IDE Editor to display UI hints (e.g. highlighted code block, color stripe in the scrollbar, etc.). When there's no "jetbrains" YAML key, the warning is registered to .gitpod.yml file, resulting in a warning at the top

Let me check with JB folks. I wonder can we have a marker in the project explorer then?

@yaohui-wyh
Copy link
Contributor Author

un-draft to move forward with CR

@yaohui-wyh yaohui-wyh marked this pull request as ready for review July 12, 2022 03:03
@yaohui-wyh yaohui-wyh requested a review from a team July 12, 2022 03:03
@akosyakov
Copy link
Member

So from JB folks we can do following on project open:

  • if a file does not exist then show a notification about issues with .gitpod.yml and suggest an action to create it and fix issues automatically
  • if it exists then we can run inspections with com.intellij.codeInspection.actions.RunInspectionIntention#rerunInspection which will open a result window

@yaohui-wyh
Copy link
Contributor Author

Hi Anton,

  • if a file does not exist then show a notification about issues with .gitpod.yml and suggest an action to create it and fix issues automatically

Notification & create-.gitpod.yml-if-missing action are already implemented at #11002. We discussed in that PR that maybe we should add inspections & quickfixes for Gitpod config schema first, so I'll just keep this one out of the Notifications part. Is that fine?

PR #11002
... If there is no .gitpod.yml in the project root, the notification popup would suggest the user to create a .gitpod.yml with generated content (with the current runtime Xmx configured)
create .gitpod.yml action


  • if it exists then we can run inspections with com.intellij.codeInspection.actions.RunInspectionIntention#rerunInspection which will open a result window

I tried and it works, thanks for the hint! I'll make a following commit.

@roboquat roboquat added size/XL and removed size/L labels Jul 17, 2022
@yaohui-wyh
Copy link
Contributor Author

Run inspection automatically on startup implemented at 49a4935

inspection-on-start.mov

@yaohui-wyh
Copy link
Contributor Author

yaohui-wyh commented Jul 17, 2022

This PR is getting larger, I'll leave some notes for ease of CR:

Known Issues:

  1. ApplyVMOptionsQuickFix is not used. It could be removed or kept in case the IDE server's VMOptions.setOption works in a Gitpod workspace. (Some background information: discussion_r909547031)
  2. description messages / quickfix names can be put into a resource bundle later. (There are lots of raw messages already in the plugin codebase, which can be refactored altogether later.)
  3. Default -Xmx size is hard coded and should be fetched from envVar.

Review needed:

  1. Inspection's problemHolder attached to .gitpod.yml PsiFile causing a "warning at a top": #issuecomment-1173504799, which I think is OK and understandable for IntelliJ IDE users. Is there any other opinions/suggestions?
  2. The Inspection description text could need some advice:

@akosyakov
Copy link
Member

akosyakov commented Jul 29, 2022

/werft run

👍 started the job as gitpod-build-yh-backend-plugin-inspection-fork.0
(with .werft/ from main)

@akosyakov
Copy link
Member

akosyakov commented Jul 29, 2022

@yaohui-wyh Sorry I did not have time to try earlier. I'm trying to build but it fails [1]:

Temp Dir: /tmp/tmp.FomdYS355A
/workspace/components/../components/ide/jetbrains/backend-plugin/src/main/resources/inspectionDescriptions/GitpodConfigInspection.html
[build|FAIL] There are some license headers missing. Please run 'leeway run components:update-license-header'.
Error Error: LICENCE_HEADER_CHECK_ONLY=true leeway run components:update-license-header || { echo "[build|FAIL] There are some license headers missing. Please run 'leeway run components:update-license-header'."; exit 1; } exit with non-zero status code.
STDOUT: 🔧  build  components:update-license-header-deps  (version 35d2fac24cf4e70444088ba9602431e7f219bbb3)
🔧  build  dev/addlicense:app                     (version 979f89794ae8aac6401def349fa432a3785dce15)

@yaohui-wyh
Copy link
Contributor Author

@yaohui-wyh Sorry I did not have time to try earlier. I'm trying to build but it fails [1]

Fixed. PTAL:)

@akosyakov
Copy link
Member

/werft run

@akosyakov
Copy link
Member

akosyakov commented Aug 11, 2022

/werft run

👍 started the job as gitpod-build-yh-backend-plugin-inspection-fork.1
(with .werft/ from main)

@akosyakov
Copy link
Member

@yaohui-wyh It's still failing: https://werft.gitpod-dev.com/job/gitpod-build-yh-backend-plugin-inspection-fork.1/raw sorry for long turnarounds

@yaohui-wyh yaohui-wyh force-pushed the yh/backend-plugin-inspection branch from 602fa25 to aac9eb3 Compare August 12, 2022 07:53
@yaohui-wyh
Copy link
Contributor Author

yaohui-wyh commented Aug 12, 2022

@yaohui-wyh It's still failing: https://werft.gitpod-dev.com/job/gitpod-build-yh-backend-plugin-inspection-fork.1/raw sorry for long turnarounds

The build failure might be caused by other components which this PR didn't touch (failed at leeway buildGo step ).
I rebased onto main and force pushed the branch, could you trigger another werft build @akosyakov ?

...
[components/common-go:lib] 
[components/common-go:lib] 
[components/common-go:lib] please gofmt your code
[components/common-go:lib] 
[components/common-go:lib] 
[components/common-go:lib] package build failed
[components/common-go:lib] Reason: exit status 1
[components/common-go:lib|FAIL] exit status 1
[components/content-service-api/go:lib] build started (version 95e5cd13ba7eb074414da24b5860f03acd7c6aa9)
☁️  uploading build artifacts to remote cache
[components/public-api/go:lib] build started (version 8f58659772b91f5c0f90b79e120981c34aed2422)
[components/public-api/go:lib] package build failed
[components/public-api/go:lib] Reason: package "components/common-go:lib" is not built
[components/public-api/go:lib|FAIL] package "components/common-go:lib" is not built
[components/content-service-api/go:lib] package build failed
[components/content-service-api/go:lib] Reason: package "components/common-go:lib" is not built
[components/content-service-api/go:lib|FAIL] package "components/common-go:lib" is not built
[components/ws-manager-api/go:lib] build started (version ff046e6126455d767e58ed0ce6137f2469ff8129)
[components/ws-daemon-api/go:lib] build started (version c9ecd6c6b08c8413c0eb83e062bdcf138daec1a6)
[components/image-builder-api/go:lib] build started (version 6b60c3b503b843c11c4ac9f9aec5f29691cc45e7)
[components/ws-manager-api/go:lib] package build failed
[components/ws-manager-api/go:lib] Reason: package "components/common-go:lib" is not built
[components/ws-manager-api/go:lib|FAIL] package "components/common-go:lib" is not built
[components/ws-daemon-api/go:lib] package build failed
[components/ws-daemon-api/go:lib] Reason: package "components/common-go:lib" is not built
[components/ws-daemon-api/go:lib|FAIL] package "components/common-go:lib" is not built
[components/image-builder-api/go:lib] package build failed
[components/image-builder-api/go:lib] Reason: package "components/common-go:lib" is not built
[components/image-builder-api/go:lib|FAIL] package "components/common-go:lib" is not built
build failed
Reason: build failed

@akosyakov
Copy link
Member

akosyakov commented Aug 12, 2022

/werft run

👍 started the job as gitpod-build-yh-backend-plugin-inspection-fork.2
(with .werft/ from main)

@yaohui-wyh
Copy link
Contributor Author

The build failure is still irrelevant to the PR (no space left for the build agent), maybe rerun werft job on another node?

...
[components/ide/jetbrains/image:intellij] failed to copy files: failed to copy directory: Error processing tar file(exit status 1): write /bin/icons/icons-v1-2.0.db: no space left on device
[components/ide/jetbrains/image:intellij] package build failed
[components/ide/jetbrains/image:intellij] Reason: exit status 1
[docker|RESULT] eu.gcr.io/gitpod-core-dev/build/ide/intellij:commit-aac9eb390aa657f74c453b2f00ad58f5ec18d4f6
[components/ide/jetbrains/image:intellij|FAIL] exit status 1
...

@akosyakov
Copy link
Member

akosyakov commented Aug 12, 2022

/werft run

👍 started the job as gitpod-build-yh-backend-plugin-inspection-fork.3
(with .werft/ from main)

@akosyakov
Copy link
Member

akosyakov commented Aug 16, 2022

/werft run

👍 started the job as gitpod-build-yh-backend-plugin-inspection-fork.4
(with .werft/ from main)

@akosyakov
Copy link
Member

akosyakov commented Aug 16, 2022

/werft run

👍 started the job as gitpod-build-yh-backend-plugin-inspection-fork.5
(with .werft/ from main)

@stale
Copy link

stale bot commented Sep 8, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Sep 8, 2022
@stale stale bot closed this Sep 20, 2022
@akosyakov akosyakov reopened this Sep 21, 2022
@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Sep 21, 2022
@akosyakov akosyakov added meta: stale This issue/PR is stale and will be closed soon meta: never-stale This issue can never become stale labels Sep 21, 2022
@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Sep 21, 2022
@akosyakov
Copy link
Member

@yaohui-wyh I've reopened it. We have some issues with build system to run tests from forks.

@andreafalzetti is looking this week in some inspections of plugins and then we will have a look at your PR to align as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants