-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Ingest Manager] Allow prerelease in package version #74452
[Ingest Manager] Allow prerelease in package version #74452
Conversation
const pkgName = pkgkey.substr(0, pkgkey.indexOf('-')); | ||
// this will return the entire string if `indexOf` return -1 | ||
const pkgVersion = pkgkey.substr(pkgkey.indexOf('-') + 1); | ||
return { pkgName, pkgVersion }; |
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.
I don't think it should accept an empty name or version.
I also think we should ensure the version is valid semver https://github.com/npm/node-semver#usage
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.
Thanks for the comment @jfsiii, how about if I check if the pkgName
is an empty string and throw an error?
For the pkgVersion
I can do the same thing, semver.valid(pkgVersion)
and if not throw an error?
Pinging @elastic/ingest-management (Team:Ingest Management) |
@@ -71,6 +71,8 @@ export const IGNORE_FILE_GLOBS = [ | |||
'x-pack/plugins/apm/e2e/**/*', | |||
|
|||
'x-pack/plugins/maps/server/fonts/**/*', | |||
// packages for the ingest manager's api integration tests could be valid semver which has dashes | |||
'x-pack/test/ingest_manager_api_integration/apis/fixtures/test_packages/**/*', |
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.
@jfsiii @skh @neptunian @nchaulet this is so we can have a valid semver as the package directory name with a dash in it. Should this only cover the packages directory? Or would it be useful if it was higher up like:
'x-pack/test/ingest_manager_api_integration/apis/fixtures/**/*
?
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.
let's start here and lift it up later if we need to
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.
Pre-commit hook changes look good.
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.
Thanks for adding the function & tests
@@ -71,6 +71,8 @@ export const IGNORE_FILE_GLOBS = [ | |||
'x-pack/plugins/apm/e2e/**/*', | |||
|
|||
'x-pack/plugins/maps/server/fonts/**/*', | |||
// packages for the ingest manager's api integration tests could be valid semver which has dashes | |||
'x-pack/test/ingest_manager_api_integration/apis/fixtures/test_packages/**/*', |
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.
let's start here and lift it up later if we need to
after(async () => { | ||
if (server.enabled) { | ||
// remove the package just in case it being installed will affect other tests | ||
await deletePackage(supertest, pkgkey); |
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.
I'd prefer to call the delete API here rather than coupling the tests to one another, but it's not a blocker
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.
Ah ok, I'll switch it back 👍
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* Allow prerelease in version * Adding integration test for prerelease version of a package * Tests for invalid package key * Removing inter-test dependency
…-task * master: (42 commits) Allow any hostname for chromium proxy bypass (elastic#74693) [ML] ML on Kibana Management: Add ability to pass a group ID filter to job management page (elastic#74533) [Metrics UI] Fix No Data preview pluralization (elastic#74399) [Bug][Security_Solution][Telemetry] Capitalize S in macOS (elastic#74688) Remove karma tests from legacy maps (elastic#74668) [Ingest Manager] stop creating events-* index pattern and placeholder index (elastic#74683) [Enterprise Search] Update the browser/document title on plugin navigation (elastic#74392) [visualizations] Add i18n translation for 'No results found' (elastic#74619) [maps] convert vector style properties to TS (elastic#74553) bump geckodriver binary to 0.27 (elastic#74638) fix: update apm agents to catch abort requests (elastic#74658) [Security Solution] Resolver children pagination (elastic#74603) add memoryStatus to df analytics page and analytics table in management (elastic#74570) [Ingest Manager] Allow prerelease in package version (elastic#74452) [App Arch]: remove legacy karma tests (elastic#74599) [i18n] revert reverted changes (elastic#74633) [Lens] Clear out all attribute properties before updating (elastic#74483) [Uptime] Fix full reloads while navigating to alert/ml (elastic#73796) Index pattern field class refactor (elastic#73180) [ML] Functional tests - stabilize DFA job type check (elastic#74631) ...
* master: (339 commits) [Ingest Node Pipelines] Sentence-case processor names (elastic#74645) Bump angular dependency from 1.7.9 to 1.8.0 (elastic#74482) [ML] Fixing schema for custom rule conditions (elastic#74676) [ML] Refactor in preparation for new es client (elastic#74552) [ML] Adding initial file analysis overrides (elastic#74376) Allow any hostname for chromium proxy bypass (elastic#74693) [ML] ML on Kibana Management: Add ability to pass a group ID filter to job management page (elastic#74533) [Metrics UI] Fix No Data preview pluralization (elastic#74399) [Bug][Security_Solution][Telemetry] Capitalize S in macOS (elastic#74688) Remove karma tests from legacy maps (elastic#74668) [Ingest Manager] stop creating events-* index pattern and placeholder index (elastic#74683) [Enterprise Search] Update the browser/document title on plugin navigation (elastic#74392) [visualizations] Add i18n translation for 'No results found' (elastic#74619) [maps] convert vector style properties to TS (elastic#74553) bump geckodriver binary to 0.27 (elastic#74638) fix: update apm agents to catch abort requests (elastic#74658) [Security Solution] Resolver children pagination (elastic#74603) add memoryStatus to df analytics page and analytics table in management (elastic#74570) [Ingest Manager] Allow prerelease in package version (elastic#74452) [App Arch]: remove legacy karma tests (elastic#74599) ...
This PR switches the parsing of package keys (e.g.
endpoint-0.13.0
) to not usesplit()
but instead look for the first occurrence of the dash delimiter.Working prerelease
Package key failures