-
Notifications
You must be signed in to change notification settings - Fork 4
TREE-417: Reworked assertion documentation #129
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
base: main
Are you sure you want to change the base?
Conversation
| - /detect/synthetic-monitoring/api-checks/assertions/ | ||
| - /detect/synthetic-monitoring/api-checks/requests/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these make sure that the old links still work? For example, the CLI repo links to various places in the documentation and we should try to avoid breaking them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was hoping for more intel on that as well as this is my first time setting up redirects.
One thing I realized is that alias doesn't seem to be a thing in Mintlify, I just updated the PR based on what Mintlify's docs say: https://www.mintlify.com/docs/create/redirects.
If there are no objections I'd verify this change first thing once its live and make adjustments if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you've set up with redirects should work. (At least, it does on the preview deployment.) Most of our redirects are set via the marketing site repo, but not sure if it matters in this case.
| - /detect/synthetic-monitoring/api-checks/assertions/ | ||
| - /detect/synthetic-monitoring/api-checks/requests/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you've set up with redirects should work. (At least, it does on the preview deployment.) Most of our redirects are set via the marketing site repo, but not sure if it matters in this case.
detect/assertions.mdx
Outdated
|
|
||
| Use this [online editor to play around](https://jsonpath.com/), or look at the examples below. We use this [jsonpath NPM | ||
| module](https://github.com/dchester/jsonpath) under the hood. | ||
| Use this [online editor](https://jsonpath.com/) to try out your own JSONPath expressions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should mention the exact RFC. https://datatracker.ietf.org/doc/rfc9535/
Maybe we want Eddie to confirm first, as mentioned in Linear.
| 2. **API Headers:** First select the header you are interested in the property field, then click "add regex". | ||
|
|
||
| Sounds more complicated than it is. Here is an example: | ||
| Here is an example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example shows the JS .match() function. We should probably remove this too, if we're removing the reference above to .match() (lines 223-224).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally missed this, thanks. I've updated the example, could use another sanity check since I'm no regex expert.
|
|
||
| Use this [online editor to play around](https://jsonpath.com/), or look at the examples below. We use this [jsonpath NPM | ||
| module](https://github.com/dchester/jsonpath) under the hood. | ||
| Use this [online editor](https://jsonpath.com/) to try out your own JSONPath expressions. For a full description of the syntax and semantics, see [RFC 9535](https://datatracker.ietf.org/doc/rfc9535/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not following the rfc and only support a subset of features. let's try to set right expectations:
- we use non-rfc .length
- we support none of the Function Extensions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are not following the rfc
@sbezludny Is a link to the RFC necessary at all then? Or do you think its worth linking it while mentioning the exceptions you highlighted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying library we're using says it follows RFC9535. 🤔
I guess it is out-dated let's then adjust it accordingly.
Affected Components
Motivation
Currently, the dev docs on Assertions live within the API checks folder: https://www.checklyhq.com/docs/detect/synthetic-monitoring/api-checks/assertions/. They contain a lot of general information on assertions which are not only valid in the context on API checks - this has led us to link to the API check docs from other check types, which is confusing.
For that reason the proposal is to have top level assertion docs which we can link to from all check types.
Implementation Notes
"Assertions" are now a top level item in the "Detect" category. I was debating moving them into "Uptime Monitoring" instead, the main issue here is that we'd be linking to it from "API checks" (Which is in the "Synthetic Monitoring" folder)
I've put redirects in place for pages that have been removed / renamed, would need to verify that these work correctly
Once this is live, we need to update the links in the web-app.