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 integration installation to data sources flyout #1561

Merged
merged 8 commits into from
Mar 19, 2024

Conversation

Swiddis
Copy link
Collaborator

@Swiddis Swiddis commented Mar 18, 2024

Description

Continuing off #1560, this PR adds the ability to install integrations from the Flyout.

Screen.Recording.2024-03-18.at.4.21.04.PM.mov

Issues Resolved

N/A

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@@ -148,9 +149,25 @@ export const InstallIntegrationFlyout = ({
),
};

const [installingIntegration, setInstallingIntegration] = useState<string | null>(null);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To allow installing in a flyout: We add the optional parameter setInstallingIntegration that is used to conditionally override the href to the setup page with an onClick that sets installingIntegration to the selected integration. (See diff in available_integration_table.tsx.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: if we switch to use navigateToApp(), do we still need this?

Reference:
Implementation of the EuiLink for datasource:

</EuiBottomBar>
</>
);
} else if (renderType === 'flyout') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bottom bar doesn't render in a flyout if we keep the existing EuiPage component, so I added multiple render types that swap the top-level components between those for an independent page or those for a flyout body. Slightly annoying but not sure what a better way is.

@Swiddis Swiddis added enhancement New feature or request integrations Used to denote items related to the Integrations project ux-integration ux related integration issues labels Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 45.00000% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 57.82%. Comparing base (31d506a) to head (e1849d2).

❗ Current head e1849d2 differs from pull request most recent head a9101bf. Consider uploading reports for the commit a9101bf to get more accurate results

Files Patch % Lines
...ents/integrations/components/setup_integration.tsx 34.78% 15 Missing ⚠️
...nage/integrations/installed_integrations_table.tsx 25.00% 5 Missing and 1 partial ⚠️
...rations/components/available_integration_table.tsx 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1561      +/-   ##
==========================================
- Coverage   57.85%   57.82%   -0.03%     
==========================================
  Files         366      366              
  Lines       13639    13666      +27     
  Branches     3555     3573      +18     
==========================================
+ Hits         7891     7903      +12     
- Misses       5687     5701      +14     
- Partials       61       62       +1     
Flag Coverage Δ
dashboards-observability 57.82% <45.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Swiddis Swiddis merged commit 6f75c96 into opensearch-project:main Mar 19, 2024
13 of 18 checks passed
@Swiddis Swiddis deleted the feature/ds-install-in-flyout branch March 19, 2024 18:36
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 19, 2024
* Allow rendering install in flyout

Signed-off-by: Simeon Widdis <[email protected]>

* Activate moving between integration browser and install view

Signed-off-by: Simeon Widdis <[email protected]>

* Lock flyout install to S3

Signed-off-by: Simeon Widdis <[email protected]>

* Autofill and lock data source name in install flyout

Signed-off-by: Simeon Widdis <[email protected]>

* Update cypress card navigation test

Signed-off-by: Simeon Widdis <[email protected]>

---------

Signed-off-by: Simeon Widdis <[email protected]>
(cherry picked from commit 6f75c96)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
RyanL1997 pushed a commit to RyanL1997/dashboards-observability that referenced this pull request Apr 18, 2024
opensearch-project#1557) (opensearch-project#1561)

* Fix bug with nextUrl using SAML and multiauth enabled

Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit f655ccf023d4266874d540478f4fb63779cebb09)

Co-authored-by: Craig Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x enhancement New feature or request integrations Used to denote items related to the Integrations project ux-integration ux related integration issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants