-
Notifications
You must be signed in to change notification settings - Fork 64
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(inventroyGroup): THEEDGE-3693- implement edge group expostion #2086
Conversation
0d5a144
to
94fec2f
Compare
/retest |
f3168d8
to
e09a571
Compare
9591394
to
aaa2cfd
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2086 +/- ##
==========================================
- Coverage 58.93% 58.50% -0.44%
==========================================
Files 183 186 +3
Lines 5871 5916 +45
Branches 1658 1663 +5
==========================================
+ Hits 3460 3461 +1
- Misses 2411 2455 +44 ☔ View full report in Codecov by Sentry. |
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
aaa2cfd
to
3f55789
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.
LGTM! Only one nitpick and good to be merged.
3f55789
to
9df1b26
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.
LGTM
/retest |
1 similar comment
/retest |
Can we also use feat instead of fix prefix for the PR title? It looks like a new feature, not a bug fix @mgold1234 |
8fa76bf
to
e9c88ad
Compare
e9c88ad
to
fb013ad
Compare
fb013ad
to
ee41e6a
Compare
hasConventionalSystems={hasConventionalSystems} | ||
hasEdgeDevices={hasEdgeDevices} |
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.
Do we really need these props here? Neither EdgeGroupsView nor InventoryGroups component accepts these two props. I tried to trace EdgeGroupsView federated module interface and found this https://github.com/RedHatInsights/edge-frontend/blob/master/src/Routes/Groups/Groups.js#L21: it only accepts locationProp, navigateProp
props.
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.
it doesnt work without that
this fixes story add support for important customer that will navigate to group screen and show edge group instead of inventory group. this pr expose Groups component from edge and check if user should see inventory or edge groups
ee41e6a
to
851e8d6
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.
LGTM! Tested locally with both enforced and standard accounts. Thank you @mgold1234!
🎉 This PR is included in version 1.57.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
this commit expose edge group component from edge,
and give the ability if important customer log in to the system and navigate to group screen, he will see edge group list instead of inventory