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 conditional installation for S3 integrations #1518

Merged
merged 12 commits into from
Mar 13, 2024

Conversation

Swiddis
Copy link
Collaborator

@Swiddis Swiddis commented Mar 11, 2024

Description

Adds rudimentary support for toggling some parts of the integration installation process. There's still some parts that need to be updated:

  • The build process on the backend still can't check if it's supposed to load saved objects/which ones. We need to include workflows as part of the build request.
  • The UI for the checkbox still needs a nicer header.
    • Since the UI is larger now it also is probably a good idea to fix the clipping with the bottom bar overlapping the content on smaller screens.
  • It'd be nice to support enabling workflows for different data source types, but this might be complex enough to warrant a different issue and PR.
  • Add workflows to remaining S3 integrations
image

Issues Resolved

Closes #1462

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.

@Swiddis Swiddis added enhancement New feature or request backport 2.x integrations Used to denote items related to the Integrations project labels Mar 11, 2024
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

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

Project coverage is 56.66%. Comparing base (d17cad3) to head (35a3bfc).

Files Patch % Lines
...ents/integrations/components/setup_integration.tsx 54.54% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1518      +/-   ##
==========================================
+ Coverage   56.62%   56.66%   +0.03%     
==========================================
  Files         348      348              
  Lines       12656    12686      +30     
  Branches     3202     3271      +69     
==========================================
+ Hits         7167     7189      +22     
- Misses       5436     5443       +7     
- Partials       53       54       +1     
Flag Coverage Δ
dashboards-observability 56.66% <72.22%> (+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.

return builtInstance;
}

getSavedObjectBundles(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the core of the backend changes -- There are several small type changes that lead to this method filtering assets based on what's actually included in the current bundles.

@@ -182,6 +184,38 @@ const runQuery = async (
}
};

export function SetupWorkflowSelector({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the main selector for the frontend, most of the other changes are wrappers around this

@@ -8,6 +8,20 @@
"labels": ["Observability", "Logs", "Flint S3"],
"author": "OpenSearch",
"sourceUrl": "https://github.com/opensearch-project/dashboards-observability/tree/main/server/adaptors/integrations/__data__/repository/nginx/info",
"workflows": [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One design choice is that workflows should be user-defined instead of something we hardcode.

const res = await addIntegrationRequest(
false,
integration.name,
config.displayName,
integration,
setCalloutLikeToast,
config.displayName,
`flint_${config.connectionDataSource}_default_${config.connectionTableName}_mview`
`flint_${config.connectionDataSource}_default_${config.connectionTableName}_mview`,
config.enabledWorkflows
Copy link
Collaborator Author

@Swiddis Swiddis Mar 11, 2024

Choose a reason for hiding this comment

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

Intentionally only enabling conditional workflows for S3 integrations and not all usages of addIntegrationRequest: #1519

Signed-off-by: Simeon Widdis <[email protected]>
@@ -282,7 +282,8 @@ export async function addIntegrationRequest(
integration: IntegrationConfig,
setToast: (title: string, color?: Color, text?: string | undefined) => void,
name?: string,
dataSource?: string
dataSource?: string,
workflows?: string[]
Copy link
Member

Choose a reason for hiding this comment

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

will these be arbitrary strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They're tied to the workflow names from the integration. They're used for workflow inclusion checking and as react keys, but I think they can be arbitrary strings (barring what React does when validating keys for rendering).

@Swiddis Swiddis merged commit 8874c8c into opensearch-project:main Mar 13, 2024
16 of 20 checks passed
@Swiddis Swiddis deleted the feature/multi-install branch March 13, 2024 17:14
opensearch-trigger-bot bot pushed a commit that referenced this pull request Mar 13, 2024
* Add workflows to integration format

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

* Render integration workflows on frontend

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

* Add ability to toggle workflows to frontend

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

* Add workflows to integration build options

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

* Add asset workflow filtering to builder

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

* Add enabled workflows to setup request

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

* Don't allow integration setup if no workflows enabled

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

* Add workflows to other integrations

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

* Improve header for workflows section

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

* Update snapshots

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

---------

Signed-off-by: Simeon Widdis <[email protected]>
(cherry picked from commit 8874c8c)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Multiple installation flows for Integrations.
3 participants