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

[Fleet] Fix asset creation during custom integration installation #174869

Merged
merged 3 commits into from
Jan 17, 2024

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Jan 15, 2024

📝 Summary

This fixes the way the asset paths are extracted since #173965. Since the assetMap is a Map, the keys can be retrieved via assetMap.keys().

@weltenwort weltenwort added Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project Team:Fleet Team label for Observability Data Collection Fleet team labels Jan 15, 2024
@weltenwort weltenwort self-assigned this Jan 15, 2024
@weltenwort weltenwort requested a review from a team as a code owner January 15, 2024 20:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Feature:EPM)

@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@weltenwort
Copy link
Member Author

Sorry about the wrong committer e-mail 🙈 I'll force-push a fixed commit.

@weltenwort weltenwort force-pushed the fleet-fix-custom-integration-assets-map-keys branch from 5539a79 to 7ebfe01 Compare January 15, 2024 20:44
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, we should probably have some tests that cover that path too do you mind creating a follow up issue for that?

@mgiota
Copy link
Contributor

mgiota commented Jan 15, 2024

@weltenwort I verify that your fix works well, so I am closing my draft PR.

Screenshot 2024-01-15 at 22 16 01

@mgiota mgiota self-requested a review January 15, 2024 21:20
Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

LGTM!

@mgiota
Copy link
Contributor

mgiota commented Jan 15, 2024

@weltenwort I did a bit more testing and as you can see when I uninstall my custom integration and reinstall it again it fails.

Screen.Recording.2024-01-15.at.22.35.49.mov

Also the Overview page of the custom integration is empty.

Screenshot 2024-01-15 at 22 25 33

This PR is approved, since it fixes the described issue. Above comments can be fixed in follow up issues. Not sure though if the uninstall/re-install issue is related only to the custom integration installation or if it is a general fleet issue. @nchaulet could weigh in here.

@nchaulet
Copy link
Member

nchaulet commented Jan 15, 2024

Not sure though if the uninstall/re-install issue is related only to the custom integration installation or if it is a general fleet issue. @nchaulet could weigh in here.

The issue seems to be only related to the custom integration as when it's uninstalled we do not have the assets anymore (that make it impossible for us to reinstall it)

I am wondering if we should just remove that install button for custom integration, or redirect somewhere else than that details page

@mgiota
Copy link
Contributor

mgiota commented Jan 15, 2024

I am wondering if we should just remove that install button for custom integration, or redirect somewhere else than that details page

Yep removing that install button sounds like a good idea to me.

@mgiota
Copy link
Contributor

mgiota commented Jan 15, 2024

@weltenwort I am reopening my PR, since your PR fixes newly created custom integrations. If the custom integration is in a broken state already, users will continue to get this error Cannot read properties of undefined (reading 'split').

After me clicking on this reinstall button, I somehow ended up in a broken state and the Installed Integrations page was empty. After me applying the initial fix, the custom integration would not appear in the Installed Integrations page, but at least I could see my rest installed Integrations and following warning on the server [WARN ][plugins.fleet] Installed package my_custom_integration 1.0.0 is not a valid package anymore. So I guess your PR didn't mean to replace my PR, just complement it and fix the root of the problem.

@weltenwort
Copy link
Member Author

weltenwort commented Jan 16, 2024

@nchaulet thanks for the review. We actually have a test that caught this, but it was circumvented in #174734 instead of fixing the problem. We're discussing steps to make sure the test always runs on the PRs that are most likely to break this functionality.

@mgiota thanks for checking. I'll investigate and possibly create a follow-up issue to disable the install button.

@yngrdyn
Copy link
Contributor

yngrdyn commented Jan 16, 2024

@weltenwort we can bring back the test to the previous form, I just saw the PR as a patch because it was blocking something important behind. wdyt?

@weltenwort
Copy link
Member Author

@yngrdyn ok, I'll try to restore it and enable it for fleet-related changes in one go

@weltenwort weltenwort requested review from a team as code owners January 16, 2024 14:31
@weltenwort
Copy link
Member Author

@mgiota @nchaulet I created a follow-up issue to remove the "(re)install" buttons/sections: #174930

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @weltenwort

Copy link
Contributor

@Ikuni17 Ikuni17 left a comment

Choose a reason for hiding this comment

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

OPs changes lgtm

@mgiota
Copy link
Contributor

mgiota commented Jan 17, 2024

@weltenwort Cool! How about the empty Overview page of the Custom Integration?

296873966-32da2a70-5db7-4d80-8f18-7c8a576f162c

Update: I created this follow up issue. If needed, feel free to update.

@weltenwort
Copy link
Member Author

@mgiota good point, thanks for creating the issue. We should definitely generate a readme too 👍

Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

LGTM

@weltenwort weltenwort merged commit 964a980 into main Jan 17, 2024
22 checks passed
@weltenwort weltenwort deleted the fleet-fix-custom-integration-assets-map-keys branch January 17, 2024 10:41
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.12 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 174869

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 17, 2024
@weltenwort
Copy link
Member Author

I don't think backports are necessary, because the change that broke this wasn't backported either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:EPM Fleet team's Elastic Package Manager (aka Integrations) project release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Observability Logs Onboarding breaks installed Integrations page
10 participants