-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Fail build if new doc snippets aren't // CONSOLE
#20402
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
Conversation
docs/reference/search.asciidoc
Outdated
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 converted these to test the ratcheting behavior locally.
javanna
left a comment
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 am not too familiar with the gradle/groovy stuff, but I like the change and I think we should get it in.
1fd1d80 to
a423745
Compare
docs/build.gradle
Outdated
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 considered reading these from a file instead but decided it wasn't worth the effort because we should convert all the files before too long, removing the need for the list.
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.
makes sense though the comment is out of date.
|
@jimferenczi and @javanna - pushed a change converting the count to a list which should make this more "git friendly". |
jimczi
left a comment
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.
@nik9000 left a very minor comment, LGTM otherwise.
a423745 to
4254aca
Compare
This tracks the snippets that probably should be converted to `// CONSOLE` or `// TESTRESPONSE` and fails the build if the list of files with such snippets doesn't match the list in `docs/build.gradle`. Setting the file looks like ``` /* List of files that have snippets that probably should be converted to * `// CONSOLE` and `// TESTRESPONSE` but have yet to be converted. Try and * only remove entries from this list. When it is empty we'll remove it * entirely and have a party! There will be cake and everything.... */ buildRestTests.expectedUnconvertedCandidates = [ 'plugins/discovery-azure-classic.asciidoc', ... 'reference/search/suggesters/completion-suggest.asciidoc', ] ``` This list is in `build.gradle` because we expect it to be fairly temporary. In a few months we'll have converted all of the docs and won't ned it any more. From now on if you add now docs that contain a snippet that shows an interaction with elasticsearch you have three choices: 1. Stick `// CONSOLE` on the interactions and `// TESTRESPONSE` on the responses. The build (specifically (`gradle docs:check`) will test that these interactions "work". If there isn't a `// TESTRESPONSE` snippet then "work" just means "Elasticsearch responds with a 200-level response code and no `WARNING` headers. This is way better than nothing. 2. Add `// NOTCONSOLE` if the snippet isn't actually interacting with Elasticsearch. This should only be required for stuff like javascript source code or `curl` against an external service like AWS or GCE. The snippet will not get "OPEN IN CONSOLE" or "COPY AS CURL" buttons or be tested. 3. Add `// TEST[skip:reason]` under the snippet. This will just skip the snippet in the test phase. This should really be reserved for snippets where we can't test them because they require an external service that we don't have at testing time. Please, please, please, please don't add more things to the list. After all, it sais there'll be cake when we remove it entirely! Relates to elastic#18160
4254aca to
156393b
Compare
|
Thanks for reviewing @javanna and @jimferenczi! I fixed the comment and merged. I'll cherry-pick into the 5.x branch now. |
|
5.x: 15a4214 |
|
@nik9000 Could you also backport this change to 5.0 branch? |
Technically I can, but I didn't want to destabilize that branch. The version conflicts every time you backport a change to the unconverted list to 5.0 are a huge pain though. |
This tracks the snippets that probably should be converted to
// CONSOLEor// TESTRESPONSEand fails the build if the listof files with such snippets doesn't match the list in
docs/build.gradle.Setting the file looks like
This list is in
build.gradlebecause we expect it to be fairlytemporary. In a few months we'll have converted all of the docs and won't
ned it any more.
From now on if you add now docs that contain a snippet that shows an
interaction with elasticsearch you have three choices:
// CONSOLEon the interactions and// TESTRESPONSEon theresponses. The build (specifically (
gradle docs:check) will test thatthese interactions "work". If there isn't a
// TESTRESPONSEsnippetthen "work" just means "Elasticsearch responds with a 200-level response
code and no
WARNINGheaders. This is way better than nothing.// NOTCONSOLEif the snippet isn't actually interacting withElasticsearch. This should only be required for stuff like javascript
source code or
curlagainst an external service like AWS or GCE. Thesnippet will not get "OPEN IN CONSOLE" or "COPY AS CURL" buttons or be
tested.
// TEST[skip:reason]under the snippet. This will just skip thesnippet in the test phase. This should really be reserved for snippets
where we can't test them because they require an external service that
we don't have at testing time.
Please, please, please, please don't add more things to the list. After
all, it sais there'll be cake when we remove it entirely!
Relates to #18160