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

Updates related_integrations field API docs #5183

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

maximpn
Copy link

@maximpn maximpn commented May 8, 2024

Relates to: elastic/kibana#173595
Epic: https://github.com/elastic/security-team/issues/1974

Summary

API updates for elastic/kibana#173595.

Updates related API docs for related_integration field changes made in elastic/kibana#178295.

@maximpn maximpn added Team: Detections/Response Detections and Response API Feature: Rules Docset: ESS Issues that apply to docs in the Stack release v8.15.0 labels May 8, 2024
@maximpn maximpn self-assigned this May 8, 2024
@maximpn maximpn requested a review from a team as a code owner May 8, 2024 10:57
@maximpn maximpn added this to In progress in Elastic Security Docs Board via automation May 8, 2024
@maximpn maximpn removed this from In progress in Elastic Security Docs Board May 8, 2024
Copy link

github-actions bot commented May 8, 2024

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

dplumlee
dplumlee previously approved these changes May 8, 2024
Copy link
Contributor

@dplumlee dplumlee left a comment

Choose a reason for hiding this comment

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

Left a few comments that apply to all the identical blocks as well but from a rules management definition pov, this looks good to me

|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:

* `package` (String, required): Integration package's name EPR uses
* `integration` (String, optional): Integration's name. It's optional for packages with the only one integration whose name matches package name but required for packages with multiple integrations.
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to sort these by required and optional, although this is a weird field where in some cases it's optional and in others it's not

Copy link
Contributor

Choose a reason for hiding this comment

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

@dplumlee Yeah, if this was strictly optional all the time, I'd say re-order the list, but since it's a little of both it's good ordered as it is.


* `package` (String, required): Integration package's name EPR uses
* `integration` (String, optional): Integration's name. It's optional for packages with the only one integration whose name matches package name but required for packages with multiple integrations.
* `version`: (String, required): Integration (package containing the integration) version constraint in https://semver.org/[semantic versioning] format. For version ranges, you must use tilde or caret syntax. For example, `~1.2.3` is from 1.2.3 to any patch version less than `1.3.0`, and `^1.2.3` is from `1.2.3`` to any minor and patch version less than `2.0.0`.
Copy link
Contributor

Choose a reason for hiding this comment

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

For example, ~1.2.3 is from 1.2.3 to any patch version less than 1.3.0, and ^1.2.3 is from 1.2.3`` to any minor and patch version less than 2.0.0`.

Do we want to display this example inline with the API field description itself or is there a better way to frame this (like a note or something)? @joepeeples

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 it's OK inline as is. I tinkered with a note or new paragraph on my local build, but it looked kinda disruptive.

Copy link

mergify bot commented May 8, 2024

This pull request is now in conflicts. Could you fix it @maximpn? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b allow-editing-related-integrations upstream/allow-editing-related-integrations
git merge upstream/main
git push upstream allow-editing-related-integrations

Copy link
Contributor

@joepeeples joepeeples left a comment

Choose a reason for hiding this comment

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

Thanks @maximpn for creating these updates! Added a few suggestions and comments. I'm not sure what's causing the conflicts in the PR but will look at that next.

@@ -380,6 +380,12 @@ Required when `actions` are used to send notifications.

* `field_names`: String[] , required

|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:
|related_integrations |Object[] a| {integrations-docs}[Elastic integrations] the rule depends on. The object has these fields:

Copy link
Author

Choose a reason for hiding this comment

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

@joepeeples,

It seems vscode doesn't properly parse integrations-docs link and shows it as is. Is there a way to verify it locally?

Copy link
Contributor

Choose a reason for hiding this comment

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

@maximpn you can build the Security docs locally if you also have elastic/docs cloned on your machine. Run the command line build alias docbldsec --open to build the Security docs, and they'll open in a browser.

That can be a bit involved, so the PR preview from CI is another way to verify. I checked and the link resolves correctly.

@@ -380,6 +380,12 @@ Required when `actions` are used to send notifications.

* `field_names`: String[] , required

|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:

* `package` (String, required): Integration package's name EPR uses
Copy link
Contributor

Choose a reason for hiding this comment

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

Some users might not know what EPR stands for, or maybe they're unfamiliar with Elastic Package Registry in general. I'm also wondering how they'd find out what a given package's name is; maybe we can add a link to help them find the integration/packages they're trying to add?

I tried looking for a list in elastic/package-registry but didn't find anything. Maybe elastic/integrations/tree/main/packages?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think we have package name documented somewhere. For example this page shows integrations list. Package's name as well as integration's name are in fact identifies and hidden from users. UI fetches integrations list from EPR (of course not directly but via Fleet) and uses that ids for API requests. To be 100% sure about id correctness users should use EPR. EPR allows to search for a package while the package contains integrations. After that package or integration ids can be saved since these a stable.

@joepeeples what do you think the best approach to describe it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@maximpn Simpler is probably better; we can link users to the EPR repo, which documents the API requests that can help them search for the right packages and integrations.

Suggested change
* `package` (String, required): Integration package's name EPR uses
* `package` (String, required): The integration package's name, as used by the https://github.com/elastic/package-registry[Elastic Package Registry].

|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:

* `package` (String, required): Integration package's name EPR uses
* `integration` (String, optional): Integration's name. It's optional for packages with the only one integration whose name matches package name but required for packages with multiple integrations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `integration` (String, optional): Integration's name. It's optional for packages with the only one integration whose name matches package name but required for packages with multiple integrations.
* `integration` (String, optional): The integration's name. This field is optional for packages with only one integration whose name matches the package name, but it's required for packages with multiple integrations.

@@ -278,6 +278,12 @@ rule's version number is incremented by 1.
`PATCH` calls enabling and disabling the rule do not increment its version
number.

|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:
|related_integrations |Object[] a| {integrations-docs}[Elastic integrations] the rule depends on. The object has these fields:

|related_integrations |Object[] a| Fleet integrations the rule depends on. The object has three fields:

* `package` (String, required): Integration package's name EPR uses
* `integration` (String, optional): Integration's name. It's optional for packages with the only one integration whose name matches package name but required for packages with multiple integrations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `integration` (String, optional): Integration's name. It's optional for packages with the only one integration whose name matches package name but required for packages with multiple integrations.
* `integration` (String, optional): The integration's name. This field is optional for packages with only one integration whose name matches the package name, but it's required for packages with multiple integrations.

Copy link

mergify bot commented May 22, 2024

This pull request is now in conflicts. Could you fix it @maximpn? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b allow-editing-related-integrations upstream/allow-editing-related-integrations
git merge upstream/main
git push upstream allow-editing-related-integrations

@maximpn maximpn force-pushed the allow-editing-related-integrations branch from acff659 to e8dbbbb Compare May 22, 2024 11:09
@maximpn
Copy link
Author

maximpn commented May 22, 2024

Hi @joepeeples,

I've updated the PR according to your comments. The last this left is to describe how to get a package name and a link to EPR documentation. Check my comment above.

@@ -286,6 +286,12 @@ rule's version number is incremented by 1.
`PATCH` calls enabling and disabling the rule do not increment its version
number.

|related_integrations |Object[] a| {integrations-docs}[Elastic integrations] the rule depends on. The object has these fields:

* `package` (String, required): Integration package's name EPR uses
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `package` (String, required): Integration package's name EPR uses
* `package` (String, required): The integration package's name, as used by the https://github.com/elastic/package-registry[Elastic Package Registry].

@maximpn
Copy link
Author

maximpn commented May 27, 2024

@joepeeples,

I've addressed your last comments. Could you review the recent changes?

@maximpn maximpn requested a review from joepeeples May 27, 2024 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Docset: ESS Issues that apply to docs in the Stack release Feature: Rules Team: Detections/Response Detections and Response v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants