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

[Ingest Manager] Change agent config YAML view to flyout #67918

Merged
merged 8 commits into from
Jun 2, 2020

Conversation

jen-huang
Copy link
Contributor

Resolves #62313

Summary

This PR moves the agent config YAML tab to be a flyout instead and address some tech debt.

  • (Tech debt) Consolidate our usage of context menus/row actions components
  • (Tech debt) Clean up of some import/exports
  • Adds agent config context menu component that includes YAML flyout, use on both config list and config details pages
  • Adds an API endpoint so that users can download YAML file from the flyout /api/ingest_manager/agent_configs/{agentConfigId}/download
    • This endpoint returns a .yml file with download headers, by default the file is named elastic-agent-config-{agentConfigId}.yml

Screenshots

image

image

image

image

@jen-huang jen-huang added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Jun 1, 2020
@jen-huang jen-huang requested a review from a team June 1, 2020 23:42
@jen-huang jen-huang self-assigned this Jun 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

const body = configToYaml(fullAgentConfig);
const headers: ResponseHeaders = {
'content-type': 'text/x-yaml',
'content-disposition': `attachment; filename="elastic-agent-config-${fullAgentConfig.id}.yml"`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruflin @ph I'm defaulting the downloaded YAML file name to elastic-agent-config-{agentConfigId}.yml but feel that it may be a bit long. Is there another convention we prefer?

Copy link
Member

Choose a reason for hiding this comment

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

The default config that is picked up by the agent is called elastic-agent.yml: https://github.com/elastic/beats/blob/master/x-pack/elastic-agent/elastic-agent.yml Taking this name would make it very easy to copy / paste. Having the id inside makes it nice for tracking but not sure we need it at the moment?

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

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.

Tested locally and it work as expected 👍
Do you think we should add a functional test for the download API endpoint? Personally I like to at least cover the green path for each endpoint

: undefined
}
items={[
<EuiContextMenuItem
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be ok if we had a URL param in the route that would trigger this flyout? Does not have to be part of this PR.
One of the integration tests I have been thinking about for Endpoint "policy" data is to ensure that data (when created/edited) lands in the right place in the YAML file, which should give us an assurance that it will make it to the Endpoint exe as intended. Having a route to get this panel would make it easier to build up that integration test.

Like I said - does not have to be part of this PR, but was wondering if you would be ok with such a change in the future when we get around to implementing that integration test 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm ok with any url params to trigger any of our flyouts :)

@jen-huang
Copy link
Contributor Author

@nchaulet I'll work on a test in a subsequent PR.

@jen-huang jen-huang merged commit eab74f4 into elastic:master Jun 2, 2020
@jen-huang jen-huang deleted the ingest/config-yaml branch June 2, 2020 23:09
jen-huang added a commit to jen-huang/kibana that referenced this pull request Jun 2, 2020
* Consolidate context menu/row action components and usage, and some export/imports

* Missed export

* Move agent config yaml tab to flyout, add and consolidate agent config action menus

* Fix i18n

* Add download config YAML endpoint and link from flyout, add common config->yaml service

* Actually remove the tab lol

* Fix i18n
jen-huang added a commit that referenced this pull request Jun 3, 2020
…8034)

* Consolidate context menu/row action components and usage, and some export/imports

* Missed export

* Move agent config yaml tab to flyout, add and consolidate agent config action menus

* Fix i18n

* Add download config YAML endpoint and link from flyout, add common config->yaml service

* Actually remove the tab lol

* Fix i18n
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ingest] Update "view agent config yaml"
6 participants