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

[Ingest Manager] Allow prerelease in package version #74452

Merged
merged 6 commits into from
Aug 10, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/dev/precommit_hook/casing_check_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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/**/*',
Copy link
Contributor Author

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/**/* ?

Copy link
Contributor

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

];

/**
Expand Down
5 changes: 3 additions & 2 deletions x-pack/plugins/ingest_manager/server/routes/epm/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
getInstallationObject,
} from '../../services/epm/packages';
import { IngestManagerError, getHTTPResponseCode } from '../../errors';
import { splitPkgKey } from '../../services/epm/registry';

export const getCategoriesHandler: RequestHandler<
undefined,
Expand Down Expand Up @@ -131,7 +132,7 @@ export const getInfoHandler: RequestHandler<TypeOf<typeof GetInfoRequestSchema.p
const { pkgkey } = request.params;
const savedObjectsClient = context.core.savedObjects.client;
// TODO: change epm API to /packageName/version so we don't need to do this
const [pkgName, pkgVersion] = pkgkey.split('-');
const { pkgName, pkgVersion } = splitPkgKey(pkgkey);
const res = await getPackageInfo({ savedObjectsClient, pkgName, pkgVersion });
const body: GetInfoResponse = {
response: res,
Expand All @@ -155,7 +156,7 @@ export const installPackageHandler: RequestHandler<
const savedObjectsClient = context.core.savedObjects.client;
const callCluster = context.core.elasticsearch.legacy.client.callAsCurrentUser;
const { pkgkey } = request.params;
const [pkgName, pkgVersion] = pkgkey.split('-');
const { pkgName, pkgVersion } = splitPkgKey(pkgkey);
try {
const res = await installPackage({
savedObjectsClient,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export async function installPackage({
force?: boolean;
}): Promise<AssetReference[]> {
// TODO: change epm API to /packageName/version so we don't need to do this
const [pkgName, pkgVersion] = pkgkey.split('-');
const { pkgName, pkgVersion } = Registry.splitPkgKey(pkgkey);
// TODO: calls to getInstallationObject, Registry.fetchInfo, and Registry.fetchFindLatestPackge
// and be replaced by getPackageInfo after adjusting for it to not group/use archive assets
const latestPackage = await Registry.fetchFindLatestPackage(pkgName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { getInstallation, savedObjectTypes } from './index';
import { deletePipeline } from '../elasticsearch/ingest_pipeline/';
import { installIndexPatterns } from '../kibana/index_pattern/install';
import { packageConfigService, appContextService } from '../..';
import { splitPkgKey } from '../registry';

export async function removeInstallation(options: {
savedObjectsClient: SavedObjectsClientContract;
Expand All @@ -21,7 +22,7 @@ export async function removeInstallation(options: {
}): Promise<AssetReference[]> {
const { savedObjectsClient, pkgkey, callCluster } = options;
// TODO: the epm api should change to /name/version so we don't need to do this
const [pkgName] = pkgkey.split('-');
const { pkgName } = splitPkgKey(pkgkey);
const installation = await getInstallation({ savedObjectsClient, pkgName });
if (!installation) throw Boom.badRequest(`${pkgName} is not installed`);
if (installation.removable === false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { AssetParts } from '../../../types';
import { pathParts } from './index';
import { pathParts, splitPkgKey } from './index';

const testPaths = [
{
Expand Down Expand Up @@ -48,3 +48,35 @@ test('testPathParts', () => {
expect(pathParts(value.path)).toStrictEqual(value.assetParts as AssetParts);
}
});

describe('splitPkgKey tests', () => {
it('throws an error if the delimiter is not found', () => {
expect(() => {
splitPkgKey('awesome_package');
}).toThrow();
});

it('throws an error if there is nothing before the delimiter', () => {
expect(() => {
splitPkgKey('-0.0.1-dev1');
}).toThrow();
});

it('throws an error if the version is not a semver', () => {
expect(() => {
splitPkgKey('awesome-laskdfj');
}).toThrow();
});

it('returns the name and version if the delimiter is found once', () => {
const { pkgName, pkgVersion } = splitPkgKey('awesome-0.1.0');
expect(pkgName).toBe('awesome');
expect(pkgVersion).toBe('0.1.0');
});

it('returns the name and version if the delimiter is found multiple times', () => {
const { pkgName, pkgVersion } = splitPkgKey('endpoint-0.13.0-alpha.1+abcd');
expect(pkgName).toBe('endpoint');
expect(pkgVersion).toBe('0.13.0-alpha.1+abcd');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import semver from 'semver';
import { Response } from 'node-fetch';
import { URL } from 'url';
import {
Expand Down Expand Up @@ -35,6 +36,26 @@ export interface CategoriesParams {
experimental?: boolean;
}

/**
* Extract the package name and package version from a string.
*
* @param pkgkey a string containing the package name delimited by the package version
*/
export function splitPkgKey(pkgkey: string): { pkgName: string; pkgVersion: string } {
// this will return an empty string if `indexOf` returns -1
const pkgName = pkgkey.substr(0, pkgkey.indexOf('-'));
if (pkgName === '') {
throw new Error('Package key parsing failed: package name was empty');
}

// this will return the entire string if `indexOf` return -1
const pkgVersion = pkgkey.substr(pkgkey.indexOf('-') + 1);
if (!semver.valid(pkgVersion)) {
throw new Error('Package key parsing failed: package version was not a valid semver');
}
return { pkgName, pkgVersion };
Copy link
Contributor

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

Copy link
Contributor Author

@jonathan-buttner jonathan-buttner Aug 6, 2020

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?

}

export const pkgToPkgKey = ({ name, version }: { name: string; version: string }) =>
`${name}-${version}`;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export default function loadTests({ loadTestFile }) {
//loadTestFile(require.resolve('./template'));
loadTestFile(require.resolve('./ilm'));
loadTestFile(require.resolve('./install_overrides'));
loadTestFile(require.resolve('./install_prerelease'));
loadTestFile(require.resolve('./install_remove_assets'));
loadTestFile(require.resolve('./install_update'));
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,32 @@
*/

import expect from '@kbn/expect';
import * as st from 'supertest';
import supertestAsPromised from 'supertest-as-promised';
import { FtrProviderContext } from '../../../api_integration/ftr_provider_context';
import { warnAndSkipTest } from '../../helpers';

export const deletePackage = async (
supertest: st.SuperTest<supertestAsPromised.Test>,
pkgkey: string
) => {
await supertest.delete(`/api/ingest_manager/epm/packages/${pkgkey}`).set('kbn-xsrf', 'xxxx');
};

export default function ({ getService }: FtrProviderContext) {
const supertest = getService('supertest');
const es = getService('es');
const dockerServers = getService('dockerServers');
const log = getService('log');

const deletePackage = async (pkgkey: string) => {
await supertest.delete(`/api/ingest_manager/epm/packages/${pkgkey}`).set('kbn-xsrf', 'xxxx');
};

const mappingsPackage = 'overrides-0.1.0';
const server = dockerServers.get('registry');

describe('installs packages that include settings and mappings overrides', async () => {
after(async () => {
if (server.enabled) {
// remove the package just in case it being installed will affect other tests
await deletePackage(mappingsPackage);
await deletePackage(supertest, mappingsPackage);
}
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { FtrProviderContext } from '../../../api_integration/ftr_provider_context';
import { warnAndSkipTest } from '../../helpers';
import { deletePackage } from './install_overrides';

export default function ({ getService }: FtrProviderContext) {
const supertest = getService('supertest');
const dockerServers = getService('dockerServers');
const log = getService('log');

const pkgkey = 'prerelease-0.1.0-dev.0+abc';
const server = dockerServers.get('registry');

describe('installs package that has a prerelease version', async () => {
after(async () => {
if (server.enabled) {
// remove the package just in case it being installed will affect other tests
await deletePackage(supertest, pkgkey);
Copy link
Contributor

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

Copy link
Contributor Author

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 👍

}
});

it('should install the package correctly', async function () {
if (server.enabled) {
await supertest
.post(`/api/ingest_manager/epm/packages/${pkgkey}`)
.set('kbn-xsrf', 'xxxx')
.expect(200);
} else {
warnAndSkipTest(this, log);
}
});
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default function ({ getService }: FtrProviderContext) {
return response.body;
};
const listResponse = await fetchPackageList();
expect(listResponse.response.length).to.be(8);
expect(listResponse.response.length).to.be(9);
} else {
warnAndSkipTest(this, log);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
- name: data_stream.type
type: constant_keyword
description: >
Data stream type.
- name: data_stream.dataset
type: constant_keyword
description: >
Data stream dataset.
- name: data_stream.namespace
type: constant_keyword
description: >
Data stream namespace.
- name: '@timestamp'
type: date
description: >
Event timestamp.
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
title: Test Dataset

type: logs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Test package

For testing a prerelease package
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
format_version: 1.0.0
name: prerelease
title: Prerelease package
description: This is a test package for testing that parsing a prerelease version works
version: 0.1.0-dev.0+abc
categories: ['security']
release: beta
type: integration
license: basic

requirement:
elasticsearch:
versions: '>7.7.0'
kibana:
versions: '>7.7.0'

icons:
- src: '/img/logo_prerelease_64_color.svg'
size: '16x16'
type: 'image/svg+xml'