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

Clarifies that expire_time is not available for endpoint exceptions #3856

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

natasha-moore-elastic
Copy link
Contributor

@natasha-moore-elastic natasha-moore-elastic commented Sep 4, 2023

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

Documentation previews:

@natasha-moore-elastic natasha-moore-elastic self-assigned this Sep 4, 2023
@natasha-moore-elastic natasha-moore-elastic requested review from dplumlee and a team September 4, 2023 11:54
@natasha-moore-elastic natasha-moore-elastic marked this pull request as ready for review September 4, 2023 11:54
@nastasha-solomon nastasha-solomon self-requested a review September 5, 2023 17:13
Copy link
Contributor

@nastasha-solomon nastasha-solomon left a comment

Choose a reason for hiding this comment

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

Left some minor suggestions, but overall LGTM! Thank you!

@@ -43,7 +43,7 @@ A JSON object with these fields:
exception queries. Boolean `AND` logic is used to evaluate the relationship
between array elements. If you want to use `OR` logic, create a separate
exception item. |Yes
|`expire_time` |String |An expiration date in ISO format. |No
|`expire_time` |String |An expiration date in ISO format. This field is not available for endpoint exceptions. |No
Copy link
Contributor

Choose a reason for hiding this comment

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

Users can likely infer that this param only works for regular exceptions, but it might be better to directly state that and remove any potential for ambiguity.

Suggested change
|`expire_time` |String |An expiration date in ISO format. This field is not available for endpoint exceptions. |No
|`expire_time` |String a|Specify when an exception item expires. The expiration date must be in ISO format.
NOTE: This field is only available for regular exception items, not endpoint exceptions. |No

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, @nastasha-solomon! I agree it's better to be explicit in this case – I'll make that change.

For the sake of consistency, I'd slightly tweak your suggestion for the first sentence, since most of the other parameter descriptions start with a noun phrase rather than an imperative verb. How does this sound:

The exception item's expiration date, in ISO format. This field is only available for regular exception items, not endpoint exceptions.

@@ -26,7 +26,7 @@ When unspecified, a new comment is created.
exception queries. Boolean `AND` logic is used to evaluate the relationship
between array elements. If you want to use `OR` logic, create a separate
exception item. |Yes
|`expire_time` |String |An expiration date in ISO format. |No
|`expire_time` |String |An expiration date in ISO format. This field is not available for endpoint exceptions. |No
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above if you decide to accept it.

Copy link
Contributor

@benironside benironside left a comment

Choose a reason for hiding this comment

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

This LGTM and I like your proposed edit based on Nastasha's comment, @natasha-moore-elastic

@natasha-moore-elastic natasha-moore-elastic merged commit 5182f16 into main Sep 8, 2023
1 check passed
@natasha-moore-elastic natasha-moore-elastic deleted the issue-3294-expire-time branch September 8, 2023 18:41
mergify bot pushed a commit that referenced this pull request Sep 8, 2023
…#3856)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)
mergify bot pushed a commit that referenced this pull request Sep 8, 2023
…#3856)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)
mergify bot pushed a commit that referenced this pull request Sep 8, 2023
…#3856)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)
mergify bot pushed a commit that referenced this pull request Sep 8, 2023
…#3856)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)
natasha-moore-elastic added a commit that referenced this pull request Sep 9, 2023
…#3856) (#3892)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)

Co-authored-by: natasha-moore-elastic <137783811+natasha-moore-elastic@users.noreply.github.com>
natasha-moore-elastic added a commit that referenced this pull request Sep 9, 2023
…#3856) (#3893)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)

Co-authored-by: natasha-moore-elastic <137783811+natasha-moore-elastic@users.noreply.github.com>
natasha-moore-elastic added a commit that referenced this pull request Sep 9, 2023
…#3856) (#3894)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)

Co-authored-by: natasha-moore-elastic <137783811+natasha-moore-elastic@users.noreply.github.com>
natasha-moore-elastic added a commit that referenced this pull request Sep 9, 2023
…#3856) (#3895)

* Clarifies that expire_time is not available for endpoint exceptions

* TW review updates

(cherry picked from commit 5182f16)

Co-authored-by: natasha-moore-elastic <137783811+natasha-moore-elastic@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] [v8.7] Create exception item API: expire_time
4 participants