-
Notifications
You must be signed in to change notification settings - Fork 115
update examples with latest request-converter #5865
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
base: main
Are you sure you want to change the base?
Conversation
|
Following you can find the validation changes against the target branch for the APIs. No changes detected. You can validate these APIs yourself by using the |
| rm output/openapi/elasticsearch-openapi-docs.tmp*.json | ||
|
|
||
| generate-language-examples: | ||
| @npm update --prefix docs/examples @elastic/request-converter |
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.
Should we pin the version in package.json/package-lock.json instead of always updating it?
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.
The version is set to the latest 9.x.x in docs/example/package.json, so the npm update just makes sure the latest release is pulled. Not sure I understand your suggestion. Are you saying we should edit package.json to have an exact version specified each time this command runs?
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.
Disclaimer: I'm not super familiar with npm.
The version is set to the latest 9.x.x in docs/example/package.json
It's set to the latest 9.1.x (which I think is what ^9.1.2 does). You probably wanted something like ^9.x instead.
so the npm update just makes sure the latest release is pulled.
It does indeed update to the latest 9.x.x (9.3.0 right now), even though the package.json file forbids it.
Are you saying we should edit package.json to have an exact version specified each time this command runs?
No, but I'm saying we should record the version we're using in the package-lock.json file, like we do for all other dependencies. This would:
- allow us to use
npm clean-installfor reproducible builds and offline installs - properly record the actual version we're using
- avoid surprises when trying to figure out the version that was used in CI
- give us a single mechanism for dependencies versions
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.
The caret means "compatible with". When you say ^9.1.2 you are saying >= 9.1.2, < 10.0.0. So package.json is correct for 9.3.0 and all future updates until we go to v10. semver docs
but I'm saying we should record the version we're using in the package-lock.json file
I agree in principle, but we do not run this command, the docs team use it. This isn't part of any automated workflows, it is pretty much one of the docs team members running this locally on their laptops to generate a fresh openapi file to upload to the docs site. If we want to enforce package-lock.json to be up to date then it will have to be them that commit it each time they use this command to generate an openapi update.
|
could we merge #5780 before? I've fixed some examples |
This PR refreshes all the language code examples. To ensure that always the latest request-converter is used for this task, I've also added an
npm update @elastic/request-converterto themake generate-language-examplestask.