-
Notifications
You must be signed in to change notification settings - Fork 4
feat(sdk): add tag APIs for python, go, and typescript #14
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -294,6 +294,50 @@ func (r *Repo) ListBranches(ctx context.Context, options ListBranchesOptions) (L | |
| return result, nil | ||
| } | ||
|
|
||
| // ListTags lists tags. | ||
| func (r *Repo) ListTags(ctx context.Context, options ListTagsOptions) (ListTagsResult, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, when the tag already exists, this method should return a concrete error type (ideally, propagated from server-side as well) that call sites can asserts in a type-safe manner instead of asserting on strings. Or has that already been taken care of on the server-side?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably fits under a similar scope as above for a separate PR that takes care of this across the sdk. Right now the other write methods do something similar. Server-side, we return a 409 with "tag already exists" which does include the information. In any case, your suggestion would be an improvement. |
||
| ttl := resolveInvocationTTL(options.InvocationOptions, defaultTokenTTL) | ||
| jwtToken, err := r.client.generateJWT(r.ID, RemoteURLOptions{Permissions: []Permission{PermissionGitRead}, TTL: ttl}) | ||
| if err != nil { | ||
| return ListTagsResult{}, err | ||
| } | ||
|
|
||
| params := url.Values{} | ||
| if options.Cursor != "" { | ||
| params.Set("cursor", options.Cursor) | ||
| } | ||
| if options.Limit > 0 { | ||
| params.Set("limit", itoa(options.Limit)) | ||
| } | ||
| if len(params) == 0 { | ||
| params = nil | ||
| } | ||
|
Comment on lines
+312
to
+314
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Is this actually necessarily? Maybe you literally copied it from other places, but I would imagine empty It would be great to have a followup PR to clean up this across the Go SDK.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I think a different PR, as you suggest, is the right answer here as the pattern is used throughout the sdk. |
||
|
|
||
| resp, err := r.client.api.get(ctx, "repos/tags", params, jwtToken, nil) | ||
| if err != nil { | ||
| return ListTagsResult{}, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| var payload listTagsResponse | ||
| if err := decodeJSON(resp, &payload); err != nil { | ||
| return ListTagsResult{}, err | ||
| } | ||
|
|
||
| result := ListTagsResult{HasMore: payload.HasMore} | ||
| if payload.NextCursor != "" { | ||
| result.NextCursor = payload.NextCursor | ||
| } | ||
| for _, tag := range payload.Tags { | ||
| result.Tags = append(result.Tags, TagInfo{ | ||
| Cursor: tag.Cursor, | ||
| Name: tag.Name, | ||
| SHA: tag.SHA, | ||
| }) | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| // ListCommits lists commits. | ||
| func (r *Repo) ListCommits(ctx context.Context, options ListCommitsOptions) (ListCommitsResult, error) { | ||
| ttl := resolveInvocationTTL(options.InvocationOptions, defaultTokenTTL) | ||
|
|
@@ -768,6 +812,80 @@ func (r *Repo) CreateBranch(ctx context.Context, options CreateBranchOptions) (C | |
| return result, nil | ||
| } | ||
|
|
||
| // CreateTag creates a tag. | ||
| func (r *Repo) CreateTag(ctx context.Context, options CreateTagOptions) (CreateTagResult, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar question to https://github.com/pierrecomputer/sdk/pull/14/changes#r3023402499 |
||
| name := strings.TrimSpace(options.Name) | ||
| if name == "" { | ||
| return CreateTagResult{}, errors.New("createTag name is required") | ||
| } | ||
| if strings.HasPrefix(name, "refs/") { | ||
| return CreateTagResult{}, errors.New("createTag name must not start with refs/") | ||
| } | ||
|
|
||
| target := strings.TrimSpace(options.Target) | ||
| if target == "" { | ||
| return CreateTagResult{}, errors.New("createTag target is required") | ||
| } | ||
|
|
||
| ttl := resolveInvocationTTL(options.InvocationOptions, defaultTokenTTL) | ||
| jwtToken, err := r.client.generateJWT(r.ID, RemoteURLOptions{Permissions: []Permission{PermissionGitWrite}, TTL: ttl}) | ||
| if err != nil { | ||
| return CreateTagResult{}, err | ||
| } | ||
|
|
||
| body := &createTagRequest{Name: name, Target: target} | ||
| resp, err := r.client.api.post(ctx, "repos/tags", nil, body, jwtToken, nil) | ||
| if err != nil { | ||
| return CreateTagResult{}, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| var payload createTagResponse | ||
| if err := decodeJSON(resp, &payload); err != nil { | ||
| return CreateTagResult{}, err | ||
| } | ||
|
|
||
| return CreateTagResult{ | ||
| Name: payload.Name, | ||
| SHA: payload.SHA, | ||
| Message: payload.Message, | ||
| }, nil | ||
| } | ||
|
|
||
| // DeleteTag deletes a tag. | ||
| func (r *Repo) DeleteTag(ctx context.Context, options DeleteTagOptions) (DeleteTagResult, error) { | ||
| name := strings.TrimSpace(options.Name) | ||
| if name == "" { | ||
| return DeleteTagResult{}, errors.New("deleteTag name is required") | ||
| } | ||
| if strings.HasPrefix(name, "refs/") { | ||
| return DeleteTagResult{}, errors.New("deleteTag name must not start with refs/") | ||
| } | ||
|
|
||
| ttl := resolveInvocationTTL(options.InvocationOptions, defaultTokenTTL) | ||
| jwtToken, err := r.client.generateJWT(r.ID, RemoteURLOptions{Permissions: []Permission{PermissionGitRead, PermissionGitWrite}, TTL: ttl}) | ||
| if err != nil { | ||
| return DeleteTagResult{}, err | ||
| } | ||
|
|
||
| body := &deleteTagRequest{Name: name} | ||
| resp, err := r.client.api.delete(ctx, "repos/tags", nil, body, jwtToken, nil) | ||
| if err != nil { | ||
| return DeleteTagResult{}, err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| var payload deleteTagResponse | ||
| if err := decodeJSON(resp, &payload); err != nil { | ||
| return DeleteTagResult{}, err | ||
| } | ||
|
|
||
| return DeleteTagResult{ | ||
| Name: payload.Name, | ||
| Message: payload.Message, | ||
| }, nil | ||
| } | ||
|
|
||
| // RestoreCommit restores a commit into a branch. | ||
| func (r *Repo) RestoreCommit(ctx context.Context, options RestoreCommitOptions) (RestoreCommitResult, error) { | ||
| targetBranch := strings.TrimSpace(options.TargetBranch) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably too late to ask (aka should have asked earlier) so don't feel free blocked on this.
I need to look up but I assume server-side we are shelling out to
gitfor listing tags, and the sorting behavior and sortable options diff a bit between some random-looking git CLI versions. I imagine we would be using the latest and the greatest (i.e., >= 2.4.9) but always good to confirm.You may take a quick peak on what options there are in https://github.com/gogs/git-module/blob/d5f694fafabb48a5c235dc5bcdd0b0548b7e42d3/repo_tag.go#L145-L157 , because I vaguely recall the default sorting behavior was pretty bizarre, in the sense that, ~impossible to do "load more" style of pagination that feels natural to human.
It is also very likely customers will soon ask about sorting capabilities otherwise they may just almost always pass
Limit: 9999or whatever so that they can sort on their end.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good thing to check. In our case, our api uses a different technique to solve the sorting problem, but I am omitting details here due to this being a public repo.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what type of details we are trying to hide here? 🤔 I think given the fact this method provides pagination with a cursor, we would have to be somewhat transparent about how are things getting sorted regardless?