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

feat: Add create and remove template tag routes #647

Merged
merged 2 commits into from
Jul 20, 2017
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
4 changes: 4 additions & 0 deletions bin/server
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ const Models = require('screwdriver-models');
const templateFactory = Models.TemplateFactory.getInstance({
datastore
});
const templateTagFactory = Models.TemplateTagFactory.getInstance({
datastore
});
const pipelineFactory = Models.PipelineFactory.getInstance({
datastore,
scm
Expand Down Expand Up @@ -120,6 +123,7 @@ datastore.setup()
notifications: notificationConfig,
ecosystem,
templateFactory,
templateTagFactory,
pipelineFactory,
jobFactory,
userFactory,
Expand Down
1 change: 1 addition & 0 deletions lib/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ module.exports = (config) => {
// Instantiating the server with the factories will apply a shallow copy
server.app = {
templateFactory: config.templateFactory,
templateTagFactory: config.templateTagFactory,
pipelineFactory: config.pipelineFactory,
jobFactory: config.jobFactory,
userFactory: config.userFactory,
Expand Down
43 changes: 31 additions & 12 deletions plugins/templates/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,37 +27,35 @@ server.register({

### Routes

#### Get all templates
`page` and `count` optional
#### Template
##### Get all templates
Copy link
Member

@tkyi tkyi Jul 20, 2017

Choose a reason for hiding this comment

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

Hm this heading style gets interesting because it looks smaller than the Arguments


`GET /templates?page={pageNumber}&count={countNumber}`
`GET /templates`

#### Get single template
##### Get a single template

You can get a single template by providing the template name and the specific version or the tag.

`GET /templates/{name}/{tag}` or `GET /templates/{name}/{version}`

**Arguments**
###### Arguments

'name', 'tag' or 'version'

* `name` - Name of the template
* `tag` - Tag of the template (e.g. `stable`, `latest`, etc)
* `version` - Version of the template
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could call out that we'll autobump versions for them when they create templates.


#### Create a template
Create a template will store the template data (`config`, `name`, `version`, `description`, `maintainer`, `labels`) into the datastore.
##### Create a template
Creating a template will store the template data (`config`, `name`, `version`, `description`, `maintainer`) into the datastore.

If the exact template and version already exist, the only thing that can be changed is `labels`.
`version` will be auto-bumped. For example, if `[email protected]` already exists and the version passed in is `1.0.0`, the newly created template will be version `1.0.1`.

If the template already exists but not the version, the new version will be stored provided that the build has correct permissions.

This endpoint is only accessible in `build` scope.
*Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that first creates the template.*

`POST /templates`

**Arguments**
###### Arguments

'name', 'version', 'description', 'maintainer', labels

Expand All @@ -83,3 +81,24 @@ Example payload:
}
}
```

#### Template Tag
Template tag allows fetching on template version by tag. For example, tag `[email protected]` as `stable`.

##### Create/Update a tag

If the template tag already exists, it will update the tag with the new version. If the template tag doesn't exist yet, this endpoint will create the tag.

*Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that creates the template.*

`PUT /templates/{templateName}/tags/{tagName}` with the following payload

* `version` - Exact version of the template (ex: `1.1.0`)

##### Delete a tag

Delete the template tag. This does not delete the template itself.

*Note: This endpoint is only accessible in `build` scope and the permission is tied to the pipeline that creates the template.*

`DELETE /templates/{templateName}/tags/{tagName}`
84 changes: 84 additions & 0 deletions plugins/templates/createTag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
'use strict';

const boom = require('boom');
const joi = require('joi');
const schema = require('screwdriver-data-schema');
const baseSchema = schema.models.templateTag.base;
const urlLib = require('url');

/* Currently, only build scope is allowed to tag template due to security reasons.
* The same pipeline that publishes the template has the permission to tag it.
*/
module.exports = () => ({
method: 'PUT',
path: '/templates/{templateName}/tags/{tagName}',
config: {
description: 'Add or update a template tag',
notes: 'Add or update a specific template',
tags: ['api', 'templates'],
auth: {
strategies: ['token', 'session'],
scope: ['build']
},
plugins: {
'hapi-swagger': {
security: [{ token: [] }]
}
},
handler: (request, reply) => {
const pipelineFactory = request.server.app.pipelineFactory;
const templateFactory = request.server.app.templateFactory;
const templateTagFactory = request.server.app.templateTagFactory;
const pipelineId = request.auth.credentials.pipelineId;
const name = request.params.templateName;
const tag = request.params.tagName;
const version = request.payload.version;

return Promise.all([
pipelineFactory.get(pipelineId),
templateFactory.get({ name, version }),
Copy link
Member

Choose a reason for hiding this comment

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

Should this be getTemplate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be get, since we want to get the exact version. And getTemplate will be refactored into string only later.

templateTagFactory.get({ name, tag })
]).then(([pipeline, template, templateTag]) => {
// If template doesn't exist, throw error
if (!template) {
throw boom.notFound(`Template ${name}@${version} not found`);
}

// If template exists, but this build's pipelineId is not the same as template's pipelineId
// Then this build does not have permission to tag the template
if (pipeline.id !== template.pipelineId) {
throw boom.unauthorized('Not allowed to tag this template');
}

// If template tag exists, then the only thing it can update is the version
if (templateTag) {
templateTag.version = version;

return templateTag.update().then(newTag => reply(newTag.toJson()).code(200));
}

// If template exists, then create the tag
return templateTagFactory.create({ name, tag, version })
.then((newTag) => {
const location = urlLib.format({
host: request.headers.host,
port: request.headers.port,
protocol: request.server.info.protocol,
pathname: `${request.path}/${newTag.id}`
});

return reply(newTag.toJson()).header('Location', location).code(201);
});
}).catch(err => reply(boom.wrap(err)));
},
validate: {
params: {
templateName: joi.reach(baseSchema, 'name'),
tagName: joi.reach(baseSchema, 'tag')
},
payload: {
version: joi.reach(baseSchema, 'version')
}
}
}
});
6 changes: 5 additions & 1 deletion plugins/templates/index.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
'use strict';

const createRoute = require('./create');
const createTagRoute = require('./createTag');
const getRoute = require('./get');
const listRoute = require('./list');
const listVersionsRoute = require('./listVersions');
const removeTagRoute = require('./removeTag');

/**
* Template API Plugin
Expand All @@ -15,9 +17,11 @@ const listVersionsRoute = require('./listVersions');
exports.register = (server, options, next) => {
server.route([
createRoute(),
createTagRoute(),
getRoute(),
listRoute(),
listVersionsRoute()
listVersionsRoute(),
removeTagRoute()
]);

next();
Expand Down
68 changes: 68 additions & 0 deletions plugins/templates/removeTag.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
'use strict';

const boom = require('boom');
const joi = require('joi');
const schema = require('screwdriver-data-schema');
const baseSchema = schema.models.templateTag.base;

/* Currently, only build scope is allowed to tag template due to security reasons.
* The same pipeline that publishes the template has the permission to tag it.
*/
module.exports = () => ({
method: 'DELETE',
path: '/templates/{templateName}/tags/{tagName}',
config: {
description: 'Delete a template tag',
notes: 'Delete a specific template',
tags: ['api', 'templates'],
auth: {
strategies: ['token', 'session'],
scope: ['build']
},
plugins: {
'hapi-swagger': {
security: [{ token: [] }]
}
},
handler: (request, reply) => {
const pipelineFactory = request.server.app.pipelineFactory;
const templateFactory = request.server.app.templateFactory;
const templateTagFactory = request.server.app.templateTagFactory;
const pipelineId = request.auth.credentials.pipelineId;
const name = request.params.templateName;
const tag = request.params.tagName;

return templateTagFactory.get({ name, tag })
.then((templateTag) => {
if (!templateTag) {
throw boom.notFound('Template tag does not exist');
}

return Promise.all([
pipelineFactory.get(pipelineId),
templateFactory.get({
name,
version: templateTag.version
})
])
.then(([pipeline, template]) => {
// Check for permission
Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a check for the template existing

Copy link
Member Author

@d2lam d2lam Jul 14, 2017

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's necessary because of our logic. templatetag can't be created if the template itself doesn't exist. There is already a check if templateTag exists. If templateTag exists, then template should exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Create template
  2. Create tag
  3. Delete template
  4. Lookup tag
  5. 500 error

Copy link
Collaborator

Choose a reason for hiding this comment

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

Deleting a template does not clean up the tags. And since you can operate on tags independently from templates, that delete operation isn't there yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deleting a template should delete the tags that are associated with it. It doesn't make sense to keep a tag that references something that doesn't exist. Just like how deleting a pipelines should delete all its jobs & builds.
Also, we don't have delete template at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add it for extra safety, but it will never reach the code with our current design.

if (pipeline.id !== template.pipelineId) {
throw boom.unauthorized('Not allowed to delete this template tag');
}

// Remove the template tag, not the template
return templateTag.remove();
});
})
.then(() => reply().code(204))
.catch(err => reply(boom.wrap(err)));
},
validate: {
params: {
templateName: joi.reach(baseSchema, 'name'),
tagName: joi.reach(baseSchema, 'tag')
}
}
}
});
Loading