Skip to content

Conversation

@jparsai
Copy link
Contributor

@jparsai jparsai commented Sep 1, 2021

Description

This PR is to introduce Stack Analysis capabilities of CRDA platform into existing Intellij Plugin of CRDA.

Documentation

Type of changes in existing files

  • Readme:- Updated details about plugin.

  • build.gradle:- Upgraded Intellij plugin version.
    Added new dependencies.

  • gradle.properties:- Bumped up lowest supported IDE version due to JCEF loading issue in 2020.2 and 2020.3 versions of Intellij.

  • GitHubRelease.java:- Downloading assets using name instead of label, as CLI releases are not having any label attached, downloading using name is not causing any issue in LSP download as well.

  • GitHubReleaseDownloader.java:- Modified logic to download file based on type (LSP or CLI) instead of introducing new class to download CLI.

  • ICookie.java:- Added new cookie for CLI.

  • Platform.java:- Introduced new methods/variables for CLI.

  • PreloadLanguageServer.java:- Sending repo name as an argument in GitHubReleaseDownloader() because now it is being used for CLI repo as well.

  • plugin.xml:- Updated plugin description which is shown in Marketplace page.
    Updated build version because we need to use 2021.x + versions of IDE due to JCEF issue.
    Added preloadingActivity, fileEditorProvider and set of Actions.

@jparsai
Copy link
Contributor Author

jparsai commented Sep 1, 2021

Hello @jeffmaury, kindly review this PR.

@jparsai jparsai force-pushed the crda-sa branch 3 times, most recently from 7ab9b9b to ec9a22f Compare September 6, 2021 15:30
@jparsai
Copy link
Contributor Author

jparsai commented Sep 6, 2021

Hello @jeffmaury, all comments are addressed PTAL.

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM.
A few remarks:

  • maybe worth to put code specific to the stack analysis in a separate package rather that in the root package
  • would be good if the editor tab displays the original file name in the addition of "Dependency Analytics Report"
  • you added a new JSON library I think IJ already provides some

@jparsai
Copy link
Contributor Author

jparsai commented Sep 7, 2021

Sure @jeffmaury I will address your comments

maybe worth to put code specific to the stack analysis in a separate package rather that in the root package
Will move files related to SA in a new package

would be good if the editor tab displays the original file name in the addition of "Dependency Analytics Report"
Please clarify my doubt, by "original file" you mean manifest file for which SA has been triggered ("Dependency Analytics Report pom.xml") or is it the temp file you are referring ("Dependency Analytics Report CRDA-1272637366849476628.sa")?

you added a new JSON library I think IJ already provides some
Let me search for it, if its there I will use that instead of adding new.

@jeffmaury
Copy link
Member

Sure @jeffmaury I will address your comments

maybe worth to put code specific to the stack analysis in a separate package rather that in the root package
Will move files related to SA in a new package

would be good if the editor tab displays the original file name in the addition of "Dependency Analytics Report"
Please clarify my doubt, by "original file" you mean manifest file for which SA has been triggered ("Dependency Analytics Report pom.xml") or is it the temp file you are referring ("Dependency Analytics Report CRDA-1272637366849476628.sa")?
The first choice ("Dependency Analytics Report pom.xml")

you added a new JSON library I think IJ already provides some
Let me search for it, if its there I will use that instead of adding new.

@yzainee-zz
Copy link

@jeffmaury So are you ok with the approach?

@jeffmaury
Copy link
Member

@jeffmaury So are you ok with the approach?

yes, waiting for the few changes and merge would follow :)

@jparsai jparsai force-pushed the crda-sa branch 2 times, most recently from 1e5beb4 to 6f55a53 Compare September 8, 2021 10:57
@jparsai
Copy link
Contributor Author

jparsai commented Sep 8, 2021

Hello @jeffmaury, I addressed your comments about Editor Tab name and individual package. Could not address the one about JSON library, I looked for it, IJ has one inbuilt but it is available in IDEA Ultimate only. PTAL

@jeffmaury
Copy link
Member

I can find Google GSON and Jackson as part of the IC library

@jparsai
Copy link
Contributor Author

jparsai commented Sep 8, 2021

I can find Google GSON and Jackson as part of the IC library

Yes, you are right, JSON library is there, I was searching for it at a wrong place. I have changed it. PTAL.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

LGTM.

Are we good to merge now @jparsai ?

@jparsai
Copy link
Contributor Author

jparsai commented Sep 13, 2021

LGTM.
Are we good to merge now @jparsai ?

Yes @jeffmaury, Please merge it.

@jeffmaury jeffmaury merged commit 4b67bd4 into redhat-developer:main Sep 13, 2021
@jeffmaury
Copy link
Member

Congrats @jparsai, great work.

@jparsai
Copy link
Contributor Author

jparsai commented Sep 13, 2021

Thank you @jeffmaury for you support and guidance.

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.

Implement full stack analysis for detailed vulnerability analysis report to align with VSCode 0.2.0 extension

3 participants