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

Fix URL documentation in API specs #44487

Merged
merged 3 commits into from
Jul 24, 2019

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Jul 17, 2019

This PR fixes the URL documentation in API specs. I checked the documentation URLs in the API specs and I applied the following changes:

@polyfractal polyfractal added the :Core/Infra/REST API REST infrastructure and utilities label Jul 17, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@Trisped
Copy link

Trisped commented Jul 18, 2019

Looks like there is a problem with rest-api-spec/src/main/resources/rest-api-spec/api/render_search_template.json
You put in "https://www.elastic.co" when it should have been "https://www.elastic.co/guide/en/elasticsearch/reference/master/search-template.html"

That is the only issue I saw with the changes.
Of course, the elasticsearch-ci/2 not building issue needs to be reviewed and I did not check if you fixed all of the URLs.

@ezimuel
Copy link
Contributor Author

ezimuel commented Jul 18, 2019

@Trisped I fixed in the second commit here 1796374. I used the specific paragraph for the render template https://www.elastic.co/guide/en/elasticsearch/reference/current/search-template.html#_validating_templates instead of the one that you proposed https://www.elastic.co/guide/en/elasticsearch/reference/current/search-template.html.

I fixed all the endpoint URLs.

@Trisped
Copy link

Trisped commented Jul 18, 2019

@ezimuel I miss read the commits (I thought newest was on top). Thanks for the feed back. Everything looks good.

@karmi
Copy link
Contributor

karmi commented Jul 19, 2019

Do we know why the patch is failing on CI?

It would be nice to get this in, so I can fold it into #42346.

@Trisped
Copy link

Trisped commented Jul 19, 2019

@karmi Not sure, the only changes were made to the documentation.
The logs states:

11:30:42 === Standard error of node node{:x-pack:qa:smoke-test-watcher:integTest-0} ===
11:30:42
11:30:42 > Task :x-pack:qa:smoke-test-watcher:integTestRunner FAILED
11:39:44
11:39:44 FAILURE: Build failed with an exception.
11:39:44
11:39:44 * What went wrong:
11:39:44 Execution failed for task ':x-pack:qa:smoke-test-watcher:integTestRunner'.
11:39:44 > There were failing tests. See the report at: file:///var/lib/jenkins/workspace/elastic+elasticsearch+pull-request-2/x-pack/qa/smoke-test-watcher/build/reports/tests/integTestRunner/index.html

There are some other errors higher up in the log, but they are not as clear to me. Since we now have merge conflicts, I would suggest merging master into this branch and see if that fixes it. Especially since the first checkin worked and the second only had the one change.

I guess I should also point out that I am not actually a dev on this. I was the one who reported the issue and @ezimuel was nice enough to make the changes.

@karmi
Copy link
Contributor

karmi commented Jul 22, 2019

Jenkins, retest this please.

@karmi
Copy link
Contributor

karmi commented Jul 22, 2019

The docs build error appears unrelated to the changes (as expected).

@rjernst rjernst requested a review from jdconrad July 24, 2019 14:23
@jdconrad
Copy link
Contributor

@elasticmachine run elasticsearch-ci/1

Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

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

LGTM!

@russcam
Copy link
Contributor

russcam commented Aug 13, 2019

Are there any plans to backport these changes to 6.x? Doing so would allow clients to update their libraries with the link changes

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.

None yet

9 participants