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

feat: Recommend dependency analytics extension #1771

Conversation

arajkumar
Copy link
Contributor

@arajkumar arajkumar commented Jan 18, 2021

@arajkumar arajkumar force-pushed the dependency-analytics-recommendation branch from 6891206 to 44cd813 Compare January 18, 2021 13:17
@fbricon fbricon requested a review from rgrunber January 18, 2021 13:19
@arajkumar arajkumar force-pushed the dependency-analytics-recommendation branch from 376792e to 6f4178e Compare January 18, 2021 13:34
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
@arajkumar arajkumar force-pushed the dependency-analytics-recommendation branch from 6f4178e to 3e32aee Compare January 19, 2021 05:56
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
@arajkumar arajkumar force-pushed the dependency-analytics-recommendation branch from f72e2a4 to 6879acd Compare January 26, 2021 06:58
Copy link
Member

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

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

Overall, looks fine. Just had one suggestion about avoiding excessive checks in the callbacks.

I also tried this on project with a large amount of poms ( https://git.eclipse.org/c/orbit/orbit-recipes.git/tree/ ) and it hit a timeout exception :

Failed to trigger application's stack analysis, try in a while. Status: 408 - REQUEST TIMEOUT

The analytics page came back empty with just the text "Unable to analyze your stack." . If this is expected in more extreme cases then I guess that's fine for now.

src/recommendation/dependencyAnalytics.ts Show resolved Hide resolved
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
@arajkumar arajkumar force-pushed the dependency-analytics-recommendation branch from 0ffd9e1 to 008101c Compare January 27, 2021 13:12
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
@arajkumar arajkumar force-pushed the dependency-analytics-recommendation branch from 008101c to 17ae046 Compare January 27, 2021 13:12
Signed-off-by: Arunprasad Rajkumar <ar.arunprasad@gmail.com>
Copy link
Member

@rgrunber rgrunber 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 addressing the issues.

I've tested each case, in addition to installing/uninstalling the analytics independent of the recommendation code. It seems to be doing the right thing each time.

Can you just squash the commits into 1 and then we can merge this.

UPDATE : I'll take care of the formatting.

src/recommendation/dependencyAnalytics.ts Show resolved Hide resolved
@rgrunber
Copy link
Member

Merged as bdacd43 .

@rgrunber rgrunber closed this Jan 28, 2021
@arajkumar
Copy link
Contributor Author

Can you just squash the commits into 1 and then we can merge this.

Usually we (in our team) prefer to do squash merge offered from Github., but I'm not sure whether it does signed commits. :)

Thanks for review this.

Screenshot 2021-01-29 at 12 29 27 AM

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.

Recommend "redhat.fabric8-analytics" extension if workspace contains "pom.xml"
3 participants