feat: Add create and remove template tag routes #647

d2lam · 2017-07-14T22:32:17Z

Context

This PR allows tagging a template, similar to how docker tagging works.

1 tag can only map to 1 version
1 version can have multiple tags

Example:

id	name	tag	version
1	mytemplate	stable	1.2.0
2	mytemplate	latest	1.2.0

Objective

Add 2 routes for template tagging: CREATE and DELETE.
UPDATE uses the same route as CREATE: send a PUT templates/{templateTag}/tags/{tagName} with the payload as: {version} instead of the usual /id, since it doesn't make sense that users need to look up the id to update.

DELETE: DELETE templates/{templateTag}/tags/{tagName}

Notes: This only allows build scope to tag template due to security reasons. It will check if the build belongs to the pipeline that publishes this template. If not, then the build is not allowed to tag the template.

References

#616

FenrirUnbound · 2017-07-14T22:58:30Z

plugins/templates/removeTag.js

+                    })
+                ])
+                .then(([pipeline, template]) => {
+                    // Check for permission


There should be a check for the template existing

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.

Create template

Create tag

Delete template

Lookup tag

500 error

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.

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.

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

tkyi · 2017-07-14T23:00:41Z

plugins/templates/createTag.js

+const schema = require('screwdriver-data-schema');
+const urlLib = require('url');
+
+/* Currently, only build scope is allowed to tag template due to security reasons.


👍 good comment

tkyi · 2017-07-14T23:05:45Z

plugins/templates/createTag.js

+                        pathname: `${request.path}/${tag.id}`
+                    });
+
+                    return reply(tag.toJson()).header('Location', location).code(201);


Hm should this return the template or the templateTag or some combination of the two?

I think it makes more sense to return just the tag. If it returns the template itself, it might be confusing to the users: "Did I just create this template?"

Hm. If the object returned has the template info AND the tag would that be more clear? Or still confusing

Generally I think it should return what GOT CREATED. That's why I don't want to include the template information since the template is not created, the tag is created. For a GET it makes sense to return the template.
What are others' opinions on this?

I think template name, version and tag together should be good enough.

No preference here

Don't mind any way you end up choosing, as long as the UI can consume it easily. If you want my opinion, I would have expected the resource that was created to be returned.

minzcmu

update README?

tkyi · 2017-07-17T22:06:48Z

plugins/templates/removeTag.js

+            const templateFactory = request.server.app.templateFactory;
+            const templateTagFactory = request.server.app.templateTagFactory;
+            const pipelineId = request.auth.credentials.pipelineId;
+            const config = request.payload;


Is it weird to pass in a payload for a DELETE?

I think it is unusual, but not forbidden according to RFC 7231.
A couple posts related to this:

https://stackoverflow.com/questions/299628/is-an-entity-body-allowed-for-an-http-delete-request

Using request body (json payload) for DELETE http requests zalando-stups/lizzy#151

I would say it's weird and we should avoid it if we can. But I think in this case, it makes sense to send a payload with it. An alternative would be delete templates/tags/{name}/{tag}? I'm not sure if that's weird either. What do you think?

I think to it would be safer for us to use the /templates/tags/{name}/{tag} endpoint instead of getting the data from the payload. Just in case other browsers drop the payload. What do you think?

The RESTful endpoint would be DELETE /templates/{name}/tags/{tagName}. You should be able to fetch the pipeline data from the template data and the JWT when comparing for authorization [build scope].

Alternatively, this could be DELETE /templates/{name}/{tagName or version} if it was less restful, but this adds a layer of confusion.

Let's go with DELETE /templates/{name}/tags/{tagName}

@petey Should the GET endpoint for template tags be GET /templates/{name}/tags/{tagName} then?

Update: keeping the GET /templates/{name}/{versionOrTag}

petey

minzcmu · 2017-07-18T17:46:11Z

plugins/templates/README.md

@@ -73,3 +73,26 @@ Example payload:
  }
 }
 ```
+
+#### Create a tag for a template version


How about update / retagging?

tkyi · 2017-07-18T19:23:04Z

Also do we do any checks to make sure the build that deletes the tag is the one that created the tag? Or can anyone create/delete a tag right now?

FenrirUnbound

👍

d2lam · 2017-07-19T17:18:23Z

@tkyi Yes it does check for permission. It's specified in the README:

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

In the code:
https://github.com/screwdriver-cd/screwdriver/blob/templatetag/plugins/templates/createTag.js#L49-L53

https://github.com/screwdriver-cd/screwdriver/blob/templatetag/plugins/templates/removeTag.js#L49-L52

d2lam · 2017-07-19T20:31:51Z

@tkyi @petey Updated with the new endpoints in this commit: 7d34294

PUT templates/{templateName}/tags/{tagName} with payload as {version}
DELETE templates/{templateName}/tags/{tagName}

tkyi · 2017-07-20T20:42:48Z

plugins/templates/README.md

-#### Get all templates
-`page` and `count` optional
+#### Template
+##### Get all templates


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

tkyi · 2017-07-20T20:53:55Z

plugins/templates/createTag.js

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


Should this be getTemplate?

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

tkyi · 2017-07-20T22:06:07Z

plugins/templates/README.md


 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


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

tkyi

feat: Add create and remove template tag routes

tkyi mentioned this pull request Jul 14, 2017

feat(616): Add template tag model screwdriver-cd/models#179

Merged

FenrirUnbound reviewed Jul 14, 2017

View reviewed changes

tkyi reviewed Jul 14, 2017

View reviewed changes

minzcmu suggested changes Jul 17, 2017

View reviewed changes

tkyi mentioned this pull request Jul 17, 2017

feat(616): Can get a template using template name and tag #646

Merged

tkyi reviewed Jul 17, 2017

View reviewed changes

petey mentioned this pull request Jul 17, 2017

Templates - Second Pass #616

Closed

16 tasks

petey approved these changes Jul 17, 2017

View reviewed changes

d2lam force-pushed the templatetag branch from 02434e1 to e082dd1 Compare July 18, 2017 17:17

minzcmu reviewed Jul 18, 2017

View reviewed changes

d2lam force-pushed the templatetag branch 2 times, most recently from e936260 to 0d1f988 Compare July 18, 2017 17:48

minzcmu approved these changes Jul 18, 2017

View reviewed changes

FenrirUnbound approved these changes Jul 19, 2017

View reviewed changes

d2lam force-pushed the templatetag branch from 95bb5f1 to 7d34294 Compare July 19, 2017 20:28

feat: Add create and remove template tag routes

f67bbbd

d2lam force-pushed the templatetag branch from bad404d to f23b51c Compare July 20, 2017 20:30

tkyi reviewed Jul 20, 2017

View reviewed changes

d2lam force-pushed the templatetag branch 2 times, most recently from 93d92be to 726ea94 Compare July 20, 2017 22:03

tkyi reviewed Jul 20, 2017

View reviewed changes

fix: change endpoints for create and remove tags

144b725

d2lam force-pushed the templatetag branch from 726ea94 to 144b725 Compare July 20, 2017 22:12

tkyi approved these changes Jul 20, 2017

View reviewed changes

tkyi merged commit 11bc128 into master Jul 20, 2017

tkyi deleted the templatetag branch July 20, 2017 22:20

d2lam pushed a commit that referenced this pull request Jul 21, 2017

Merge pull request #647 from screwdriver-cd/templatetag

24a9309

feat: Add create and remove template tag routes

jithine mentioned this pull request Dec 23, 2022

[Snyk] Security upgrade jsonwebtoken from 8.5.1 to 9.0.0 #2807

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add create and remove template tag routes #647

feat: Add create and remove template tag routes #647

d2lam commented Jul 14, 2017 •

edited

Loading

FenrirUnbound Jul 14, 2017

d2lam Jul 14, 2017 •

edited

Loading

FenrirUnbound Jul 17, 2017

FenrirUnbound Jul 17, 2017

d2lam Jul 17, 2017

d2lam Jul 17, 2017

tkyi Jul 14, 2017

tkyi Jul 14, 2017

d2lam Jul 14, 2017

tkyi Jul 15, 2017 •

edited

Loading

d2lam Jul 17, 2017

minzcmu Jul 17, 2017

tkyi Jul 17, 2017

FenrirUnbound Jul 19, 2017

minzcmu left a comment

tkyi Jul 17, 2017

d2lam Jul 18, 2017

tkyi Jul 18, 2017

petey Jul 18, 2017

d2lam Jul 19, 2017 •

edited

Loading

tkyi Jul 19, 2017 •

edited

Loading

petey left a comment

minzcmu Jul 18, 2017

tkyi commented Jul 18, 2017

FenrirUnbound left a comment

d2lam commented Jul 19, 2017

d2lam commented Jul 19, 2017

tkyi Jul 20, 2017 •

edited

Loading

tkyi Jul 20, 2017

d2lam Jul 20, 2017

tkyi Jul 20, 2017

tkyi left a comment •

edited

Loading

feat: Add create and remove template tag routes #647

feat: Add create and remove template tag routes #647

Conversation

d2lam commented Jul 14, 2017 • edited Loading

Context

Objective

References

Choose a reason for hiding this comment

d2lam Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkyi Jul 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minzcmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d2lam Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

tkyi Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

petey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkyi commented Jul 18, 2017

FenrirUnbound left a comment

Choose a reason for hiding this comment

d2lam commented Jul 19, 2017

d2lam commented Jul 19, 2017

tkyi Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkyi left a comment • edited Loading

Choose a reason for hiding this comment

d2lam commented Jul 14, 2017 •

edited

Loading

d2lam Jul 14, 2017 •

edited

Loading

tkyi Jul 15, 2017 •

edited

Loading

d2lam Jul 19, 2017 •

edited

Loading

tkyi Jul 19, 2017 •

edited

Loading

tkyi Jul 20, 2017 •

edited

Loading

tkyi left a comment •

edited

Loading