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

fix: Fix template resolution in UI #2754

Merged
merged 1 commit into from
Apr 20, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions api/openapi-spec/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -2863,52 +2863,52 @@
"title": "SubmitOpts are workflow submission options",
"properties": {
"dryRun": {
"description": "DryRun validates the workflow on the client-side without creating it.\nThis option is not supported in API.",
"type": "boolean",
"format": "boolean"
"format": "boolean",
Copy link
Member Author

Choose a reason for hiding this comment

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

All of the changes in *.go and *.swagger.json files are only docstring changes.

"title": "DryRun validates the workflow on the client-side without creating it. This option is not supported in API"
},
"entrypoint": {
"description": "Entrypoint overrides spec.entrypoint in io.argoproj.workflow.v1alpha1.",
"type": "string"
"type": "string",
"title": "Entrypoint overrides spec.entrypoint"
},
"generateName": {
"description": "GenerateName overrides metadata.generateName in io.argoproj.workflow.v1alpha1.",
"type": "string"
"type": "string",
"title": "GenerateName overrides metadata.generateName"
},
"instanceID": {
"description": "InstanceID adds controller's instance id label in io.argoproj.workflow.v1alpha1.",
"type": "string"
"type": "string",
"title": "InstanceID binds the Resource to the specified instance ID"
},
"labels": {
"description": "Labels adds metadata.labels in io.argoproj.workflow.v1alpha1.",
"type": "string"
"type": "string",
"title": "Labels adds to metadata.labels"
},
"name": {
"description": "Name overrides metadata.name in io.argoproj.workflow.v1alpha1.",
"type": "string"
"type": "string",
"title": "Name overrides metadata.name"
},
"ownerReference": {
"description": "OwnerReference creates the metadata.ownerReference in io.argoproj.workflow.v1alpha1.",
"title": "OwnerReference creates a metadata.ownerReference",
"$ref": "#/definitions/io.k8s.api.core.v1.OwnerReference"
},
"parameterFile": {
"description": "ParameterFile holds parameter file path.\nThis option is not supported in API.",
"type": "string"
"type": "string",
"title": "ParameterFile holds a reference to a parameter file. This option is not supported in API"
},
"parameters": {
"description": "Parameters passes input parameters to io.argoproj.workflow.v1alpha1.",
"type": "array",
"title": "Parameters passes input parameters to workflow",
"items": {
"type": "string"
}
},
"serverDryRun": {
"description": "ServerDryRun validates the workflow on the Server-side without creating it.",
"type": "boolean",
"format": "boolean"
"format": "boolean",
"title": "ServerDryRun validates the workflow on the server-side without creating it"
},
"serviceAccount": {
"description": "ServiceAccount runs all pods in the workflow using specified serviceaccount.",
"description": "ServiceAccount runs all pods in the workflow using specified ServiceAccount.",
"type": "string"
}
}
Expand Down
22 changes: 11 additions & 11 deletions pkg/apiclient/workflow/workflow.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1746,52 +1746,52 @@
"properties": {
"name": {
"type": "string",
"description": "Name overrides metadata.name in workflow."
"title": "Name overrides metadata.name"
},
"generateName": {
"type": "string",
"description": "GenerateName overrides metadata.generateName in workflow."
"title": "GenerateName overrides metadata.generateName"
},
"instanceID": {
"type": "string",
"description": "InstanceID adds controller's instance id label in workflow."
"title": "InstanceID binds the Resource to the specified instance ID"
},
"entrypoint": {
"type": "string",
"description": "Entrypoint overrides spec.entrypoint in workflow."
"title": "Entrypoint overrides spec.entrypoint"
},
"parameters": {
"type": "array",
"items": {
"type": "string"
},
"description": "Parameters passes input parameters to workflow."
"title": "Parameters passes input parameters to workflow"
},
"parameterFile": {
"type": "string",
"description": "ParameterFile holds parameter file path.\nThis option is not supported in API."
"title": "ParameterFile holds a reference to a parameter file. This option is not supported in API"
},
"serviceAccount": {
"type": "string",
"description": "ServiceAccount runs all pods in the workflow using specified serviceaccount."
"description": "ServiceAccount runs all pods in the workflow using specified ServiceAccount."
},
"dryRun": {
"type": "boolean",
"format": "boolean",
"description": "DryRun validates the workflow on the client-side without creating it.\nThis option is not supported in API."
"title": "DryRun validates the workflow on the client-side without creating it. This option is not supported in API"
},
"serverDryRun": {
"type": "boolean",
"format": "boolean",
"description": "ServerDryRun validates the workflow on the Server-side without creating it."
"title": "ServerDryRun validates the workflow on the server-side without creating it"
},
"labels": {
"type": "string",
"description": "Labels adds metadata.labels in workflow."
"title": "Labels adds to metadata.labels"
},
"ownerReference": {
"$ref": "#/definitions/k8s.io.apimachinery.pkg.apis.meta.v1.OwnerReference",
"description": "OwnerReference creates the metadata.ownerReference in workflow."
"title": "OwnerReference creates a metadata.ownerReference"
}
},
"title": "SubmitOpts are workflow submission options"
Expand Down
24 changes: 11 additions & 13 deletions pkg/apis/workflow/v1alpha1/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,28 +30,26 @@ type TemplateReferenceHolder interface {

// SubmitOpts are workflow submission options
type SubmitOpts struct {
// Name overrides metadata.name in workflow.
// Name overrides metadata.name
Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
// GenerateName overrides metadata.generateName in workflow.
// GenerateName overrides metadata.generateName
GenerateName string `json:"generateName,omitempty" protobuf:"bytes,2,opt,name=generateName"`
// InstanceID adds controller's instance id label in workflow.
// InstanceID binds the Resource to the specified instance ID
InstanceID string `json:"instanceID,omitempty" protobuf:"bytes,3,opt,name=instanceID"`
// Entrypoint overrides spec.entrypoint in workflow.
// Entrypoint overrides spec.entrypoint
Entrypoint string `json:"entryPoint,omitempty" protobuf:"bytes,4,opt,name=entrypoint"`
// Parameters passes input parameters to workflow.
// Parameters passes input parameters to workflow
Parameters []string `json:"parameters,omitempty" protobuf:"bytes,5,rep,name=parameters"`
// ParameterFile holds parameter file path.
// This option is not supported in API.
// ParameterFile holds a reference to a parameter file. This option is not supported in API
ParameterFile string `json:"parameterFile,omitempty" protobuf:"bytes,6,opt,name=parameterFile"`
// ServiceAccount runs all pods in the workflow using specified serviceaccount.
// ServiceAccount runs all pods in the workflow using specified ServiceAccount.
ServiceAccount string `json:"serviceAccount,omitempty" protobuf:"bytes,7,opt,name=serviceAccount"`
// DryRun validates the workflow on the client-side without creating it.
// This option is not supported in API.
// DryRun validates the workflow on the client-side without creating it. This option is not supported in API
DryRun bool `json:"dryRun,omitempty" protobuf:"varint,8,opt,name=dryRun"`
// ServerDryRun validates the workflow on the Server-side without creating it.
// ServerDryRun validates the workflow on the server-side without creating it
ServerDryRun bool `json:"serverDryRun,omitempty" protobuf:"varint,9,opt,name=serverDryRun"`
// Labels adds metadata.labels in workflow.
// Labels adds to metadata.labels
Labels string `json:"labels,omitempty" protobuf:"bytes,10,opt,name=labels"`
// OwnerReference creates the metadata.ownerReference in workflow.
// OwnerReference creates a metadata.ownerReference
OwnerReference *metav1.OwnerReference `json:"ownerReference,omitempty" protobuf:"bytes,11,opt,name=ownerReference"`
}
24 changes: 11 additions & 13 deletions pkg/apis/workflow/v1alpha1/generated.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

77 changes: 77 additions & 0 deletions ui/src/app/shared/template-resolution.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import * as models from '../../models';
import {ResourceScope, WorkflowStep} from '../../models';

export function getResolvedTemplates(workflow: models.Workflow, node: models.NodeStatus): models.Template {
Copy link
Contributor

Choose a reason for hiding this comment

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

this function has a lot of logic in it - so we should unit test it, but it is also a lift-and-shift, so not today

let tmpTemplate = {
template: node.templateName,
templateRef: node.templateRef
} as WorkflowStep;
const scope = getTemplateScope(node);
const referencedTemplates: models.Template[] = [];
let resolvedTemplate: models.Template = null;
const maxDepth = 10;
for (let i = 1; i < maxDepth + 1; i++) {
const templRef = resolveTemplateReference(scope.ResourceScope, scope.ResourceName, tmpTemplate);
let tmpl = null;
if (templRef.StorageNeeded) {
tmpl = workflow.status.storedTemplates[templRef.StoredTemplateName];
} else if (tmpTemplate.template) {
tmpl = workflow.spec.templates.find(item => item.name === tmpTemplate.template);
}
if (!tmpl) {
const name = templRef.StoredTemplateName || tmpTemplate.template;
// tslint:disable-next-line: no-console
console.error(`StoredTemplate ${name} not found`);
return undefined;
}
referencedTemplates.push(tmpl);
if (!tmpl.template && !tmpl.templateRef) {
break;
}
tmpTemplate = tmpl;
if (i === maxDepth) {
// tslint:disable-next-line: no-console
console.error(`Template reference too deep`);
return undefined;
}
}
referencedTemplates.reverse().forEach(tmpl => {
tmpl = Object.assign({}, tmpl);
delete tmpl.template;
delete tmpl.templateRef;
resolvedTemplate = Object.assign({}, resolvedTemplate, tmpl);
});
return resolvedTemplate;
}

// resolveTemplateReference resolves the stored template name of a given template holder on the template scope and determines
// if it should be stored
function resolveTemplateReference(callerScope: ResourceScope, resourceName: string, caller: WorkflowStep): {StoredTemplateName: string; StorageNeeded: boolean} {
if (caller.templateRef) {
// We are calling an external WorkflowTemplate or ClusterWorkflowTemplate. Template storage is needed
// We need to determine if we're calling a WorkflowTemplate or a ClusterWorkflowTemplate
const referenceScope: ResourceScope = caller.templateRef.clusterScope ? 'cluster' : 'namespaced';
const name = referenceScope + '/' + caller.templateRef.name + '/' + caller.templateRef.template;
return {StoredTemplateName: name, StorageNeeded: true};
} else if (callerScope !== 'local') {
// Either a WorkflowTemplate or a ClusterWorkflowTemplate is calling a template inside itself. Template storage is needed
const name = callerScope + '/' + resourceName + '/' + caller.template;
return {StoredTemplateName: name, StorageNeeded: true};
} else {
// A Workflow is calling a template inside itself. Template storage is not needed
return {StoredTemplateName: '', StorageNeeded: false};
}
}

function getTemplateScope(nodeStatus: models.NodeStatus): {ResourceScope: ResourceScope; ResourceName?: string} {
// For compatibility: an empty TemplateScope is a local scope
if (!nodeStatus.templateScope) {
return {ResourceScope: 'local'};
}
const split = nodeStatus.templateScope.split('/');
// For compatibility: an unspecified ResourceScope in a TemplateScope is a namespaced scope
if (split.length === 1) {
return {ResourceScope: 'namespaced', ResourceName: split[0]};
}
return {ResourceScope: split[0] as ResourceScope, ResourceName: split[1]};
}
50 changes: 1 addition & 49 deletions ui/src/app/shared/utils.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import {Observable} from 'rxjs';
import * as models from '../../models';
import {NODE_PHASE, Template} from '../../models';
import {NODE_PHASE} from '../../models';

export const Utils = {
statusIconClasses(status: string): string {
Expand Down Expand Up @@ -38,54 +38,6 @@ export const Utils = {
return Observable.from([val as T]);
},

getResolvedTemplates(workflow: models.Workflow, node: models.NodeStatus): models.Template {
let tmpTemplate = {
template: node.templateName,
templateRef: node.templateRef
} as Template;
let scope = node.templateScope;
const referencedTemplates: models.Template[] = [];
let resolvedTemplate: models.Template;
const maxDepth = 10;
for (let i = 1; i < maxDepth + 1; i++) {
let storedTemplateName = '';
if (tmpTemplate.templateRef) {
storedTemplateName = `${tmpTemplate.templateRef.name}/${tmpTemplate.templateRef.template}`;
scope = tmpTemplate.templateRef.name;
} else {
storedTemplateName = `${scope}/${tmpTemplate.template}`;
}
let tmpl = null;
if (scope && storedTemplateName) {
tmpl = workflow.status.storedTemplates[storedTemplateName];
} else if (tmpTemplate.template) {
tmpl = workflow.spec.templates.find(item => item.name === tmpTemplate.template);
}
if (!tmpl) {
// tslint:disable-next-line: no-console
console.error(`StoredTemplate ${storedTemplateName} not found`);
return undefined;
}
referencedTemplates.push(tmpl);
if (!tmpl.template && !tmpl.templateRef) {
break;
}
tmpTemplate = tmpl;
if (i === maxDepth) {
// tslint:disable-next-line: no-console
console.error(`Template reference too deep`);
return undefined;
}
}
referencedTemplates.reverse().forEach(tmpl => {
tmpl = Object.assign({}, tmpl);
delete tmpl.template;
delete tmpl.templateRef;
resolvedTemplate = Object.assign({}, resolvedTemplate, tmpl);
});
return resolvedTemplate;
},

tryJsonParse(input: string) {
try {
return (input && JSON.parse(input)) || null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as models from '../../../../models';
import {Timestamp} from '../../../shared/components/timestamp';
import {ResourcesDuration} from '../../../shared/resources-duration';
import {services} from '../../../shared/services';
import {getResolvedTemplates} from '../../../shared/template-resolution';
import {Utils} from '../../../shared/utils';

require('./workflow-node-info.scss');
Expand Down Expand Up @@ -184,7 +185,7 @@ export class WorkflowNodeContainers extends React.Component<Props, {selectedSide
}

public render() {
const template = Utils.getResolvedTemplates(this.props.workflow, this.props.node);
const template = getResolvedTemplates(this.props.workflow, this.props.node);
if (!template || (!template.container && !template.script)) {
return (
<div className='white-box'>
Expand Down
Loading