Skip to content

Conversation

@jen-huang
Copy link
Contributor

@jen-huang jen-huang commented Apr 29, 2020

Resolves #57959

Summary

This PR implements functionality to edit an agent config by using the existing form component for creating an agent config in the Settings tab of config details page:

image

  • There are some differences from the create agent config view: name and description fields are wrapped in a description, no system metrics option, no advanced settings toggle, and inclusion of Delete config button
  • Default config cannot be deleted, so the delete button for default config is disabled
  • Configs cannot be deleted if active agents are still enrolled into them
  • Delete agent config API endpoint was changed to be single delete, and prevents deletion when agents are enrolled (i.e. both UI and API enforces agent enrollment state)

Screenshots

Expand me for more screenshots

.
Save changes bottom bar appears after form is dirty:

image

Save changes modal when agents are enrolled (no modal is shown if no agents are enrolled):

image

Success notification:

image

Delete config modal when no agents are enrolled:

image

Delete config modal when agents are enrolled:

image

Delete disabled for default config:

image

@jen-huang jen-huang added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Apr 29, 2020
@jen-huang jen-huang self-assigned this Apr 29, 2020
@elasticmachine
Copy link
Contributor

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

@jen-huang jen-huang changed the title [Ingest] Edit agent config UI [Ingest] Agent config settings UI Apr 30, 2020
@jen-huang jen-huang marked this pull request as ready for review April 30, 2020 04:17
@jen-huang jen-huang requested a review from a team April 30, 2020 04:17
@jen-huang jen-huang requested a review from a team as a code owner April 30, 2020 04:17
Copy link
Contributor

@jfsiii jfsiii left a comment

Choose a reason for hiding this comment

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

I have only reviewed on my phone, but it looks very good.

<EuiButtonEmpty
color="ghost"
onClick={() => {
setAgentConfig({ ...originalAgentConfig });
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why do we make a copy here?

const refreshConfig = useConfigRefresh();
const [isNavDrawerLocked, setIsNavDrawerLocked] = useState(false);
const [agentConfig, setAgentConfig] = useState<AgentConfig>({
...originalAgentConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be a copy of the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really, just a personal habit when working with objects that are passed by reference (though in this context with useState it doesn't matter much)

public async delete(
soClient: SavedObjectsClientContract,
ids: string[],
unassignFromAgentConfigs: boolean = true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should go into the options parameter.

We can still default it to true/whatever and it's backwards compatible.

This reminded me of https://ariya.io/2011/08/hall-of-api-shame-boolean-trap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adjusted

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Contributor

@hbharding hbharding left a comment

Choose a reason for hiding this comment

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

lgtm

@jen-huang jen-huang merged commit 8a30462 into elastic:master Apr 30, 2020
@jen-huang jen-huang deleted the ingest/config-settings branch April 30, 2020 18:12
jen-huang added a commit to jen-huang/kibana that referenced this pull request Apr 30, 2020
* Remove duplicate donut chart, move used donut chart closer to usage, clean up import paths

* Change delete agent config to only single delete and delete all its associated data sources

* Initial pass at settings tab

* Reuse existing create agent config form instead

* Fix delete api

* Prevent nav drawer from hiding bottom bar (save action area) content

* Remove delete config functionality from list page

* Prevent API from deleting config with agents enrolled

* Fix namespace populating in form

* Display confirmation modal to deploy to agents if agents are detected

* Adjust confirm delete copy

* Fix i18n checks

* Fix type check

* Fix it again

* De-dupe confirm modal

* Fix i18n

* Update agent config info after saving

* Adjust skip unassign from agent config option schema in delete datasource method
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels May 4, 2020
jen-huang added a commit that referenced this pull request May 4, 2020
* Remove duplicate donut chart, move used donut chart closer to usage, clean up import paths

* Change delete agent config to only single delete and delete all its associated data sources

* Initial pass at settings tab

* Reuse existing create agent config form instead

* Fix delete api

* Prevent nav drawer from hiding bottom bar (save action area) content

* Remove delete config functionality from list page

* Prevent API from deleting config with agents enrolled

* Fix namespace populating in form

* Display confirmation modal to deploy to agents if agents are detected

* Adjust confirm delete copy

* Fix i18n checks

* Fix type check

* Fix it again

* De-dupe confirm modal

* Fix i18n

* Update agent config info after saving

* Adjust skip unassign from agent config option schema in delete datasource method

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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.8.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Ingest] Agent configuration settings tab UI

5 participants