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

Add a new tab to the EntityPage for the Quarkus console #1154

Closed
wants to merge 3 commits into from

Conversation

cmoulliard
Copy link

@cmoulliard cmoulliard commented Apr 3, 2024

Description

This PR:

  • Add a new tab to the ./packages/app/src/components/catalog/EntityPage/index.tsx file in order to allow to display the Quarkus console
  • Enhance the documentation page to better explain how to run locally the dynamic plugins with yarn start
  • Add a Table of Contents to the dynamic-plugins.md file

Screenshot 2024-04-03 at 10 19 36

Which issue(s) does this PR fix

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

Signed-off-by: cmoulliard <cmoulliard@redhat.com>
Signed-off-by: cmoulliard <cmoulliard@redhat.com>
@cmoulliard cmoulliard requested a review from a team as a code owner April 3, 2024 08:15
Copy link
Contributor

github-actions bot commented Apr 3, 2024

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1154!

Copy link
Member

@davidfestal davidfestal left a comment

Choose a reason for hiding this comment

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

2 remarks here:

  • Do you think you could create a dedicated PR for the Docs enhancement ?
  • With dynamic plugins, we should not require changing the source code of the frontend application in order to wire a dynamic plugin. That seems like an anti-pattern. Users of the showcase that will want to bring their own plugins in a dedicated tab will not be able to change the showcase image . @tumido @gashcrumb Any idea about how to contribute a new tab ? I assume we should have something, right ?

@davidfestal
Copy link
Member

davidfestal commented Apr 3, 2024

@tumido @gashcrumb Any idea about how to contribute a new tab ? I assume we should have something, right ?

And if not the it seems to me it would be quite urgent to open an issue and plan it.

@cmoulliard
Copy link
Author

cmoulliard commented Apr 3, 2024

  • That seems like an anti-pattern.

I fully agree with you but my PR has been created based on discussions with @gashcrumb as currently this is the only way to add a tab

@cmoulliard
Copy link
Author

  • Do you think you could create a dedicated PR for the Docs enhancement ?

I can do that but then it will be needed to remove commits pushed on this PR ....

@cmoulliard
Copy link
Author

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1154!

I did a test on our cluster using the image of the PR and that works too.

@davidfestal
Copy link
Member

I can do that but then it will be needed to remove commits pushed on this PR ....

I assume this PR should be rebased yes, or possibly replaced b the new one.

BTW I don't really see the point in adding a TOC, since one is already constructed automatically by GH (as would be in other markdown viewers) based on the Markown structure:

Screenshot from 2024-04-03 11-22-04

@davidfestal
Copy link
Member

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1154!

I did a test on our cluster using the image of the PR and that works too.

My main problem is that it seems to me that we should not need to create a new container image in order to add a tab specific to a dynamic plugin.

Signed-off-by: cmoulliard <cmoulliard@redhat.com>
@cmoulliard
Copy link
Author

BTW I don't really see the point in adding a TOC, since one is already constructed automatically by GH (as would be in other markdown viewers) based on the Markown structure:

I removed the TOC from this PR

@tumido
Copy link
Member

tumido commented Apr 3, 2024

So... The code is completely correct and valid.

Secondly, you should really think twice before adding new tabs. The fact that you can't just add a new tab by the dynamic plugins mechanism is 100% intentional. Our aim was to always reduce the amount of tabs we provide. We don't want to end up in a situation where we have a new tab per plugin.

Each tab should represent a purpose that is generic and common to every entity of the same kind and type in the catalog. That's why we don't have a distinct GitHub Actions tab and Tekton tab -> we have a single CI tab instead.

The presence of a new tab should be a PM/UX question, this is not something we should decide only on code quality/review basis.

@cmoulliard
Copy link
Author

cmoulliard commented Apr 3, 2024

Each tab should represent a purpose that is generic and common to every entity of the same kind and type in the catalog. That's why we don't have a distinct GitHub Actions tab and Tekton tab -> we have a single CI tab instead.

The presence of a new tab should be a PM/UX question, this is not something we should decide only on code quality/review basis.

I agree with you and that will discussed soon between the Quarkus's Product Manager and RHDH team.

FYI: The Quarkus Console plugin could become (if approved by the different Runtime PMs) a runtime plugin supporting: Quarkus, Spring Boot, EAP, etc java runtimes

@davidfestal
Copy link
Member

The fact that you can't just add a new tab by the dynamic plugins mechanism is 100% intentional

I think that even this assertion should be validated by PM / UX.
It might be that the availability of such an option (to place a dynamic plugin in a new tab) would still be required, even though not the default or the recommended way.

cc @christophe-f

Copy link

openshift-ci bot commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from davidfestal. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.1% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

github-actions bot commented Apr 3, 2024

The image is available at: quay.io/janus-idp/backstage-showcase:pr-1154!

@christophe-f
Copy link
Contributor

Like Tom mentioned, I agree that we should not ship too many tabs by default. 
With that said, a user should be able to create, remove, rename tabs as they wish and not be limited. We should definitely provide this feature.

We've been focusing a lot on the Component kind where most of the tabs are already present, but if someone would like to add more tab to the user, groups or custom Kind, currently they are not allowed.

@davidfestal
Copy link
Member

Issue #1160 was opened for this purpose,to avoid fixing this with a plugin-specific fix.

@openshift-merge-robot
Copy link

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@invincibleJai
Copy link
Member

Issue #1160 was opened for this purpose,to avoid fixing this with a plugin-specific fix.

@cmoulliard I see #1173 is merged and the associated issue is closed

so do we still need this PR or it can be closed?

cc @gashcrumb

@davidfestal
Copy link
Member

davidfestal commented Jun 13, 2024

so do we still need this PR or it can be closed?

No we don't need this PR anymore and it should be closed, since the corresponding required dynamic plugin support has been added in PR #1173

@gashcrumb
Copy link
Member

Yep, this approach shouldn't be necessary anymore, so let's close this.

@gashcrumb gashcrumb closed this Jun 13, 2024
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.

Add new tab EntityPage for the QuarkusConsole page
7 participants