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

Bug 1878700: Fix Add Pipeline Trigger for Pipeline Operator 1.1 #6612

Merged
merged 1 commit into from
Sep 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions frontend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@
"@types/react-router-dom": "5.1.2",
"@types/react-transition-group": "2.x",
"@types/react-virtualized": "9.x",
"@types/semver": "^6.0.0",
"@types/webpack": "4.x",
"babel-loader": "^8.1.0",
"bootstrap-sass": "^3.3.7",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ import { RouteModel } from '@console/internal/models';
import { EventListenerModel, TriggerTemplateModel } from '../../../../models';
import { Pipeline, PipelineRun } from '../../../../utils/pipeline-augment';
import { PIPELINE_SERVICE_ACCOUNT } from '../../const';
import { getPipelineOperatorVersion } from '../../utils/pipeline-operator';
import {
TriggerBindingKind,
EventListenerKind,
EventListenerKindBindingReference,
TriggerBindingKind,
TriggerTemplateKind,
TriggerTemplateKindParam,
} from '../../resource-types';
Expand All @@ -29,10 +31,33 @@ export const createTriggerTemplate = (
};
};

export const createEventListener = (
export const createEventListener = async (
namespace: string,
triggerBindings: TriggerBindingKind[],
triggerTemplate: TriggerTemplateKind,
): EventListenerKind => {
): Promise<EventListenerKind> => {
const pipelineOperatorVersion = await getPipelineOperatorVersion(namespace);

const mapTriggerBindings: (
triggerBinding: TriggerBindingKind,
) => EventListenerKindBindingReference = (triggerBinding: TriggerBindingKind) => {
// The Tekton CRD `EventListeners` before Tekton Triggers 0.5 requires a name
// instead of a ref here to link `TriggerBinding` or `ClusterTriggerBinding`.
if (
pipelineOperatorVersion?.major === 0 ||
(pipelineOperatorVersion?.major === 1 && pipelineOperatorVersion?.minor === 0)
) {
return {
kind: triggerBinding.kind,
name: triggerBinding.metadata.name,
} as EventListenerKindBindingReference;
}
return {
kind: triggerBinding.kind,
ref: triggerBinding.metadata.name,
};
};

return {
apiVersion: apiVersionForModel(EventListenerModel),
kind: EventListenerModel.kind,
Expand All @@ -43,7 +68,7 @@ export const createEventListener = (
serviceAccountName: PIPELINE_SERVICE_ACCOUNT,
triggers: [
{
bindings: triggerBindings.map(({ kind, metadata: { name } }) => ({ kind, name })),
bindings: triggerBindings.map(mapTriggerBindings),
template: { name: triggerTemplate.metadata.name },
},
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ export const submitTrigger = async (
pipelineRun,
triggerTemplateParams,
);
const eventListener: EventListenerKind = createEventListener(
const eventListener: EventListenerKind = await createEventListener(
thisNamespace,
[triggerBinding.resource],
triggerTemplate,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,16 @@ export type TriggerTemplateKind = K8sResourceCommon & {
};

export type EventListenerKindBindingReference = {
// TriggerBinding / ClusterTriggerBinding name reference
name: string;
kind?: string;
// TriggerBinding / ClusterTriggerBinding reference
kind: string;
// Ref is used since Tekton Triggers 0.5 (part of OpenShift Pipeline Operator 1.1)
ref: string;
// We also support older operators, so need to show & save the old field as well.
// https://github.com/tektoncd/triggers/pull/603/files
// https://github.com/tektoncd/triggers/releases/tag/v0.5.0 and
// https://github.com/tektoncd/triggers/releases/tag/v0.6.0
/** @deprecated use ref instead */
name?: string;
Comment on lines +33 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we might be able to use keyof here, or would it become too verbose? Especially since name is deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know what the solution looks like then. But the usage in triggers.ts works fine with name and at least VScode shows these deprecated hint. But I'm open to see and try an example here. /me open the TS keyof definition :)

};

export type EventListenerKindTrigger = {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
import { k8sList } from '@console/internal/module/k8s';
import { ClusterServiceVersionKind } from '@console/operator-lifecycle-manager';
import { getPipelineOperatorVersion } from '../pipeline-operator';

jest.mock('@console/internal/module/k8s', () => ({
k8sList: jest.fn(),
}));

beforeEach(() => {
jest.resetAllMocks();
});

const k8sListMock = k8sList as jest.Mock;

describe('getPipelineOperatorVersion', () => {
it('should fetch the ClusterServiceVersion from the api', async () => {
const csvs = [
{
metadata: { name: 'openshift-pipelines-operator.v1.0.1' },
spec: { version: '1.0.1' },
status: { phase: 'Succeeded' },
} as ClusterServiceVersionKind,
];
k8sListMock.mockReturnValueOnce(Promise.resolve(csvs));

const version = await getPipelineOperatorVersion('unit-test');

expect(version.raw).toBe('1.0.1');
expect(version.major).toBe(1);
expect(version.minor).toBe(0);
expect(version.patch).toBe(1);
expect(k8sList).toHaveBeenCalledTimes(1);
});

it('should return the active ClusterServiceVersion if multiple returns', async () => {
const csvs = [
{
metadata: { name: 'openshift-pipelines-operator.v1.0.1' },
spec: { version: '1.0.1' },
status: { phase: 'Succeeded' },
} as ClusterServiceVersionKind,
{
metadata: { name: 'openshift-pipelines-operator.v1.1.1' },
spec: { version: '1.1.1' },
status: { phase: 'Pending' },
} as ClusterServiceVersionKind,
];
k8sListMock.mockReturnValueOnce(Promise.resolve(csvs));

const version = await getPipelineOperatorVersion('unit-test');

expect(version.raw).toBe('1.0.1');
expect(version.major).toBe(1);
expect(version.minor).toBe(0);
expect(version.patch).toBe(1);
expect(k8sList).toHaveBeenCalledTimes(1);
});

it('should fetch the latest (highest) ClusterServiceVersion from the api', async () => {
const csvs = [
{
metadata: { name: 'openshift-pipelines-operator.v1.0.1' },
spec: { version: '1.0.1' },
status: { phase: 'Succeeded' },
} as ClusterServiceVersionKind,
{
metadata: { name: 'openshift-pipelines-operator.v10.11.12' },
spec: { version: '10.11.12' },
status: { phase: 'Succeeded' },
} as ClusterServiceVersionKind,
{
metadata: { name: 'openshift-pipelines-operator.v1.1.1' },
spec: { version: '1.1.1' },
status: { phase: 'Succeeded' },
} as ClusterServiceVersionKind,
];
k8sListMock.mockReturnValueOnce(Promise.resolve(csvs));

const version = await getPipelineOperatorVersion('unit-test');

expect(version.raw).toBe('10.11.12');
expect(version.major).toBe(10);
expect(version.minor).toBe(11);
expect(version.patch).toBe(12);
expect(k8sList).toHaveBeenCalledTimes(1);
});

it('should return null if there is no ClusterServiceVersion available', async () => {
k8sListMock.mockReturnValueOnce(Promise.resolve([]));
await expect(getPipelineOperatorVersion('unit-test')).resolves.toBe(null);
expect(k8sList).toHaveBeenCalledTimes(1);
});

it('should return null if there is no pipeline operator at all', async () => {
const csvs = [
{
metadata: { name: 'another-operator' },
spec: { version: '1.0.0' },
status: { phase: 'Succeeded' },
} as ClusterServiceVersionKind,
];
k8sListMock.mockReturnValueOnce(Promise.resolve(csvs));
await expect(getPipelineOperatorVersion('unit-test')).resolves.toBe(null);
expect(k8sList).toHaveBeenCalledTimes(1);
});

it('should return null if there is no matching ClusterServiceVersion available', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you write one more test for valid items but not including a Pipeline Operator? I think you covered both sides (none and mixed), but I think there is still valid test possible here for just invalid ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

added new test, it should return null if there is no pipeline operator at all

const csvs = [
{
metadata: { name: 'another-operator' },
spec: { version: '1.0.0' },
status: { phase: 'Succeeded' },
} as ClusterServiceVersionKind,
{
metadata: { name: 'openshift-pipelines-operator.v1.1.1' },
spec: { version: '1.1.1' },
status: { phase: 'Installing' },
} as ClusterServiceVersionKind,
];
k8sListMock.mockReturnValueOnce(Promise.resolve(csvs));
await expect(getPipelineOperatorVersion('unit-test')).resolves.toBe(null);
expect(k8sList).toHaveBeenCalledTimes(1);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { compare, parse, SemVer } from 'semver';
import {
ClusterServiceVersionKind,
ClusterServiceVersionModel,
ClusterServiceVersionPhase,
} from '@console/operator-lifecycle-manager';
import { k8sList } from '@console/internal/module/k8s';

export const getPipelineOperatorVersion = async (namespace: string): Promise<SemVer | null> => {
const allCSVs: ClusterServiceVersionKind[] = await k8sList(ClusterServiceVersionModel, {
ns: namespace,
});
const matchingCSVs = allCSVs.filter(
(csv) =>
csv.metadata?.name?.startsWith('openshift-pipelines-operator') &&
csv.status?.phase === ClusterServiceVersionPhase.CSVPhaseSucceeded,
Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewballantyne @christianvogt Does it makes sense to check the status.phase of the Pipeline Operator to get the right version? If this is not required I could use k8sListPartialMetadata insteadof k8sList which makes the API response much smaller.

);
const versions = matchingCSVs.map((csv) => parse(csv.spec.version)).filter(Boolean);
// Orders from small (oldest) to highest (newest) version
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this comment vs the tests.

Suggested change
// Orders from small (oldest) to highest (newest) version
// Orders versions from latest to earliest

You're grabbing the last one, the earliest version based on your tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

The order after this versions.sort(compare) is 1.0, 1.1, 2.0, ... And then I selected the last one (to get the newest version). The comment was only for the versions.sort line. I will update that comment so that it matches both lines or do some code change. don't know yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I was confused by your test... I didn't read the phase value. You're right. Apologizes -- ignore this thread.

versions.sort(compare);
if (versions.length > 0) {
return versions[versions.length - 1];
}
return null;
};
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,9 @@ export const getEventListenerTriggerBindingNames = (
): ResourceModelLink[] =>
bindings?.map((binding) => ({
model: getResourceModelFromBindingKind(binding.kind),
name: binding.name,
// Ref is used since Tekton Triggers 0.5 (OpenShift Pipeline Operator 1.1)
// We keep the fallback to name here to support also OpenShift Pipeline Operator 1.0.
name: binding.ref || binding.name,
}));

export const getTriggerTemplatePipelineName = (triggerTemplate: TriggerTemplateKind): string => {
Expand Down
11 changes: 2 additions & 9 deletions frontend/public/module/k8s/cluster-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,20 +168,13 @@ export const getOpenShiftVersion = (cv: ClusterVersionKind): string => {
return lastUpdate.state === 'Partial' ? `Updating to ${lastUpdate.version}` : lastUpdate.version;
};

type ParsedVersion = {
major: number;
minor: number;
patch: number;
prerelease: string[];
};

export const getCurrentVersion = (cv: ClusterVersionKind): string => {
return _.get(cv, 'status.history[0].version') || _.get(cv, 'spec.desiredUpdate.version');
};

export const getReportBugLink = (cv: ClusterVersionKind): { label: string; href: string } => {
const version: string = getCurrentVersion(cv);
const parsed: ParsedVersion = semver.parse(version);
const parsed = semver.parse(version);
if (!parsed) {
return null;
}
Expand Down Expand Up @@ -214,7 +207,7 @@ export const getReleaseNotesLink = (version: string): string => {
return null;
}

const parsed: ParsedVersion = semver.parse(version);
const parsed = semver.parse(version);
if (!parsed) {
return null;
}
Expand Down
5 changes: 5 additions & 0 deletions frontend/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2707,6 +2707,11 @@
resolved "https://registry.yarnpkg.com/@types/selenium-webdriver/-/selenium-webdriver-3.0.16.tgz#50a4755f8e33edacd9c406729e9b930d2451902a"
integrity sha512-lMC2G0ItF2xv4UCiwbJGbnJlIuUixHrioOhNGHSCsYCJ8l4t9hMCUimCytvFv7qy6AfSzRxhRHoGa+UqaqwyeA==

"@types/semver@^6.0.0":
version "6.2.2"
resolved "https://registry.yarnpkg.com/@types/semver/-/semver-6.2.2.tgz#5c27df09ca39e3c9beb4fae6b95f4d71426df0a9"
integrity sha512-RxAwYt4rGwK5GyoRwuP0jT6ZHAVTdz2EqgsHmX0PYNjGsko+OeT4WFXXTs/lM3teJUJodM+SNtAL5/pXIJ61IQ==

"@types/shallowequal@^1.1.1":
version "1.1.1"
resolved "https://registry.yarnpkg.com/@types/shallowequal/-/shallowequal-1.1.1.tgz#aad262bb3f2b1257d94c71d545268d592575c9b1"
Expand Down