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

Console-ify curl statements for allocation explain API docs #23190

Merged
merged 5 commits into from Feb 16, 2017

Conversation

dakrone
Copy link
Member

@dakrone dakrone commented Feb 15, 2017

Relates to #23001

@dakrone dakrone added >docs General docs changes v5.3.1 v6.0.0-alpha1 labels Feb 15, 2017
@@ -17,12 +17,20 @@ To explain the allocation of a shard, issue a request:

[source,js]
--------------------------------------------------
$ curl -XGET 'http://localhost:9200/_cluster/allocation/explain' -d '{
PUT /myindex
Copy link
Member

Choose a reason for hiding this comment

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

I'd either call this out in the prose like the painless docs do for their example or I'd surround this in block comments (////) with an explanation that it is hidden but makes the snippets on the page make more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I added some surrounding prose

@@ -35,12 +43,12 @@ specified as either the node id or node name.

[source,js]
--------------------------------------------------
$ curl -XGET 'http://localhost:9200/_cluster/allocation/explain' -d '{
Copy link
Member

Choose a reason for hiding this comment

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

I think this is missing the GET /_cluster/allocation/explain part?

Copy link
Member Author

Choose a reason for hiding this comment

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

It moved further down under the // TESTSETUP part

Copy link
Member

Choose a reason for hiding this comment

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

Er, sorry, posted to the wrong spot.

@@ -35,12 +43,12 @@ specified as either the node id or node name.

[source,js]
--------------------------------------------------
$ curl -XGET 'http://localhost:9200/_cluster/allocation/explain' -d '{
Copy link
Member

Choose a reason for hiding this comment

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

Er, sorry, posted to the wrong spot.

@@ -35,12 +43,12 @@ specified as either the node id or node name.

[source,js]
--------------------------------------------------
$ curl -XGET 'http://localhost:9200/_cluster/allocation/explain' -d '{
{
Copy link
Member

Choose a reason for hiding this comment

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

I think this is missing the GET /_cluster/allocation/explain part.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how to test this since I don't know the name or ID of a node currently in the cluster to which the shard is assigned

Copy link
Member Author

Choose a reason for hiding this comment

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

Which is why I made it a JSON blob instead of a console blob

Copy link
Member

Choose a reason for hiding this comment

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

I'd make it a CONSOLE blob and explicitly script the test with // TEST[skip:don't know the name or id of a node currently in the cluster]. You could probably get one the same way we do with yaml tests but that seems like a lot of work. Maybe worth it if we find this snippet has an error, but otherwise not. This way we get the COPY AS CURL and VIEW IN CONSOLE links which are handy, even if we do skip the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I added that

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Thanks!

@dakrone dakrone merged commit 5443f7d into elastic:master Feb 16, 2017
@dakrone dakrone added the v5.4.0 label Feb 16, 2017
dakrone added a commit that referenced this pull request Feb 16, 2017
* Console-ify curl statements for allocation explain API docs

Relates to #23001

* Fix tests

* Remove exclusion from build.gradle

* Call out index creation in prose

* Add console back and skip test
dakrone added a commit that referenced this pull request Feb 16, 2017
* Console-ify curl statements for allocation explain API docs

Relates to #23001

* Fix tests

* Remove exclusion from build.gradle

* Call out index creation in prose

* Add console back and skip test
@dakrone dakrone deleted the alloc-explain-consolify-docs branch May 24, 2017 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants