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

Resurrect _xpack/license routes #73930

Merged
merged 2 commits into from
Jun 9, 2021

Conversation

joegallo
Copy link
Contributor

@joegallo joegallo commented Jun 8, 2021

Related to #51816 / #68905.

Adds back the _xpack/license routes when using rest compatibility for a request.

@sethmlarson sethmlarson added the Team:Clients Meta label for clients team label Jun 8, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/clients-team (Team:Clients)

@jakelandis
Copy link
Contributor

I don't think this will impact the client's team. When the _xpack/* paths were removed they were almost all (maybe all of them) were removed for 7.0.0 from the spec, but still left in the code as deprecated. We are not re-introducing _xpack/* to the spec for 7.x, rather we are including the custom specs that have the _xpack/* defined in a custom v7compatible source set (directory) to allow for testing that shouldn't have any impact on the actual spec or the clients.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM - 1 minor non blocking comment.

task.replaceKeyInDo("license.post_start_basic", "xpack-license.post_start_basic")
task.replaceKeyInDo("license.post_start_trial", "xpack-license.post_start_trial")

task.addAllowedWarningRegex(".*_xpack/license.* is deprecated.*")
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be allowed or just warnings ? I think we can require it since the modified spec only has the deprecated variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh, good point. I'll check!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I looked into this for a little while, and I don't think just warnings ends up working very well for this, at least as it's currently written.

It requires a test name (see the InjectWarnings constructor), which works well enough on its own, but we also (sometimes) need to allow warnings on the do sections within setup or teardown, and that happens to end up being the case here.

That's not to say that we couldn't rework the machinery a bit to make that possible, but it's more than just a tweak on this PR.

Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM

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.

5 participants