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

[DOCS] Unset machine learning upgrade mode #39149

Merged
merged 3 commits into from
Feb 20, 2019

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Feb 19, 2019

Related to #38876 and #38993

This PR makes the close-ml.asciidoc file set upgrade mode to false before it finishes.

@lcawl lcawl added >docs General docs changes v7.0.0 :ml Machine learning v6.7.0 v8.0.0 v7.2.0 labels Feb 19, 2019
@lcawl lcawl requested a review from droberts195 February 19, 2019 20:23
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

[source,js]
--------------------------------------------------
POST /_ml/set_upgrade_mode?enabled=false
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 we'd be better off clearing this with // TEARDOWN in this case. Another option would be to teach ESRestTestCase to clear it between tests, but I don't think that is needed here because we don't commonly set this in tests.

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 @nik9000 I've made that change

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need POST /_ml/set_upgrade_mode?enabled=true to still be a user visible snippet. Only POST /_ml/set_upgrade_mode?enabled=false should be a secret TEARDOWN snippet. (I guess this is also complicated because close-ml.asciidoc gets included in other docs files, and I'm not sure how the TEARDOWNs interact when there are inclusions.)

Copy link
Contributor Author

@lcawl lcawl Feb 20, 2019

Choose a reason for hiding this comment

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

@droberts195 You can preview the changes here:
The snippet is still visible; have I misunderstood your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, yes, the preview looks fine.

Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -25,8 +25,9 @@ prevent new jobs from opening, use the <<ml-set-upgrade-mode,set upgrade mode AP
POST _ml/set_upgrade_mode?enabled=true
--------------------------------------------------
// CONSOLE
// TEARDOWN
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 transforms this snippet from a test into a thing that modifies tests on this page. But there aren't tests on this page. So I think this just makes this snippet not run.

I think something like

////////////
Take us out of upgrade mode after running any snippets on this page.

[source,js]
--------------------------------------------------
POST _ml/set_upgrade_mode?enabled=false
--------------------------------------------------
// CONSOLE
// TEARDOWN
////////////

Will run the snippet and always clean up.

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.

❤️

@lcawl lcawl merged commit bdd3a82 into elastic:master Feb 20, 2019
@lcawl lcawl deleted the disable-upgrad-mode branch February 20, 2019 20:03
weizijun pushed a commit to weizijun/elasticsearch that referenced this pull request Feb 22, 2019
weizijun pushed a commit to weizijun/elasticsearch that referenced this pull request Feb 22, 2019
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.

6 participants