-
Notifications
You must be signed in to change notification settings - Fork 65
plugin for viewing and applying RedHat Insights recommendations #837
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
plugin for viewing and applying RedHat Insights recommendations #837
Conversation
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ydayagi it requires minor changes. I left some comments to help you address the issues.
f2763cc
to
c8811ed
Compare
workspaces/redhat-resource-optimization/plugins/redhat-resource-optimization/package.json
Outdated
Show resolved
Hide resolved
614ba76
to
be4580d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this creates a new redhat-resource-optimization
workspace in this repository. How does this relate to the workspace (redhat-resource-optimization) in the Backstage Community Plugins repository?
Before merging I'd just like to understand what the plan is, and whether it needs to be removed from Backstage Community Plugins to avoid having two divergent versions of the same plugin?
sorry for the late response. just finished preparing the workspace with links to redhat. |
36faf88
to
a059a29
Compare
@ydayagi, the plugin already exists in the Backstage Community Plugins (workspace) with Red Hat codeowners listed as active maintainers. If there is no intention to maintain the version published in the community, can we please ensure the workspace is removed from there? The risk is that things like shared dependency update PRs that touch this workspace may become stuck waiting on review from those codeowners (so it can impact other maintainers in the project). |
sure. i will work on it and update you since it is ready |
@BethGriggs here is the PR for removing from community plugins backstage/community-plugins#4185 |
a059a29
to
773e03f
Compare
@BethGriggs sonarCloud complains about code duplicity. can we ignore/bypass it? I did not find any duplicate code in the files it reports |
Concern about the plugin living in two locations resolved
@ydayagi, thanks for following up with the removal in the other repository. Dismissed my review as I just wanted to ensure the plugin didn't continue to exist in two places. I'm personally not familiar with SonarCloud setup here, so may need someone else to comment on that. |
7157afa
to
db1795f
Compare
29a9010
to
6c7543b
Compare
Just make sure to resolve the SonarQube code duplication issues and this should be ready! |
I have no idea how to do that. it is not clear where is the duplication and why it thinks there is a duplication |
Signed-off-by: Yaron Dayagi <[email protected]>
750774c
to
0da863a
Compare
@schultzp2020 Most of the duplicity is reported for files in the packages folder. Other plugins have the same file with similar content. I think that when the plugins were introduced for the first time, the SonarCloud report was ignored. |
@nickboldt @davidfestal can u help me with sonarCloud? |
c2e699f
to
0da863a
Compare
seems like issues raised have been addressed.
Assuming that Paul is cool to merge this despite the slightly high dupe report from sonarqube cloud; with followup work TBD in https://issues.redhat.com/browse/RHIDP-7881 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
New changes are detected. LGTM label has been removed. |
|
Hey, I just made a Pull Request!
✔️ Checklist