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

[API] GetRelease by tag only return release #14397

Merged

Conversation

cameronbraid
Copy link
Contributor

get release by tag should filter out tag releases to be consistent with list releases and get by id

There is a inconsistency between the 'list releases for repo' and 'get release by tag'.

The former filters out any release that is_tag=true where as the later includes them.

So there is no consistent way to identify if a release exists for a given tag name.

If I iterate the 'list releases for repo' results it wont be there. If I use 'get release by tag' it will be there but won't have any fields like title/notes/commitssh

@lafriks lafriks added this to the 1.14.0 milestone Jan 19, 2021
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 19, 2021
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 19, 2021
@cameronbraid
Copy link
Contributor Author

Can I also backport this to release/v1.13 ?

@CirnoT
Copy link
Contributor

CirnoT commented Jan 19, 2021

Makes it even more inconsistent due to DELETE being used to remove tag.

Your solution is sound however it would require:

  • Removing DELETE on ​/repos​/{owner}​/{repo}​/releases​/tags​/{tag} (having endpoint that removes tag grouped under release seems counter-intuitive anyway, release is not a tag nor can release have more than one tag)
  • Adding GET for /repos/{owner}/{repo}/tags/{tag} to fetch specific tag
  • Adding DELETE for /repos​/{owner}​/{repo}​/tags/{tag} to remove specific tag

@codecov-io
Copy link

codecov-io commented Jan 19, 2021

Codecov Report

Merging #14397 (76da070) into master (0c0445c) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14397      +/-   ##
==========================================
+ Coverage   41.84%   41.86%   +0.01%     
==========================================
  Files         744      744              
  Lines       79779    79782       +3     
==========================================
+ Hits        33387    33400      +13     
+ Misses      40874    40866       -8     
+ Partials     5518     5516       -2     
Impacted Files Coverage Δ
routers/api/v1/repo/release_tags.go 48.38% <0.00%> (-5.19%) ⬇️
services/pull/check.go 48.52% <0.00%> (-0.74%) ⬇️
services/pull/pull.go 42.64% <0.00%> (+0.49%) ⬆️
models/repo_list.go 78.76% <0.00%> (+0.88%) ⬆️
modules/queue/workerpool.go 60.81% <0.00%> (+2.04%) ⬆️
modules/git/tree_entry_nogogit.go 93.75% <0.00%> (+6.25%) ⬆️
modules/util/timer.go 85.71% <0.00%> (+42.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c0445c...76da070. Read the comment docs.

@cameronbraid
Copy link
Contributor Author

Not sure what you want conceptually for how gitea wants release/tags to function

From my limited understanding, a tag can exist without a release - however there is still a row in the release table for said tag

A release can be created for a tag which upgrades the row in the release table to be is_tag = false and fills on extra information

I assumes that the GET/DELETE on /repos​/{owner}​/{repo}​/releases​/tags​/{tag} were alternative endpoints for GET/DELETE on /repos/{owner}/{repo}/releases/{id} but doing the lookups buy tag name instead of release id

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 19, 2021
@CirnoT
Copy link
Contributor

CirnoT commented Jan 19, 2021

I assumes that the GET/DELETE on /repos​/{owner}​/{repo}​/releases​/tags​/{tag} were alternative endpoints for GET/DELETE on /repos/{owner}/{repo}/releases/{id} but doing the lookups buy tag name instead of release id

No, the DELETE endpoint works only if the release is is_tag=true and is used to remove tag via git itself:

if err := releaseservice.DeleteReleaseByID(release.ID, ctx.User, true); err != nil {

if delTag {
if stdout, err := git.NewCommand("tag", "-d", rel.TagName).
SetDescription(fmt.Sprintf("DeleteReleaseByID (git tag -d): %d", rel.ID)).
RunInDir(repo.RepoPath()); err != nil && !strings.Contains(err.Error(), "not found") {
log.Error("DeleteReleaseByID (git tag -d): %d in %v Failed:\nStdout: %s\nError: %v", rel.ID, repo, stdout, err)
return fmt.Errorf("git tag -d: %v", err)
}

By changing how GET endpoint works, you make it inconsistent with how DELETE behaves (note it can't be used to delete release!)

if !release.IsTag {
ctx.Error(http.StatusConflict, "IsTag", errors.New("a tag attached to a release cannot be deleted directly"))
return

Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

this is a breaking change!
and should at first be considdered carefully!

@6543
Copy link
Member

6543 commented Jan 20, 2021

was added at #12932 related: #13358

@6543 6543 added modifies/api This PR adds API routes or modifies them pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! labels Jan 20, 2021
@6543
Copy link
Member

6543 commented Jan 20, 2021

Summary of my review:

the #13358 is not jet been released ....

so we could make DELETE /repos/{owner}/{repo}/releases/tags/{tag} to be used as "delete release by tag"
we then have to add a delete git tag endpoint

I would suggest DELETE /repos/{owner}/{repo}/tags/{tag}
and add the check suggested by this pull ...

@cameronbraid ☝️ can you ajust this pull at least to the last one ... then I'll file a pull up myselve ... or you could go for the whole thing ;)

@johanvdw
Copy link
Contributor

It seems the github api is also filtering out tags without release:
https://api.github.com/repos/go-gitea/gitea/releases/tags/v0.9.9
vs
https://api.github.com/repos/go-gitea/gitea/releases/tags/v1.0.0

@6543
Copy link
Member

6543 commented Feb 4, 2021

I'll adjust this pull & send a follow up ...

@6543 6543 changed the title get release by tag should filter out tag releases [API] GetRelease by tag only return release Feb 4, 2021
@6543
Copy link
Member

6543 commented Feb 4, 2021

🚀

@6543 6543 merged commit 3c965c3 into go-gitea:master Feb 4, 2021
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Feb 9, 2021
* master: (22 commits)
  Add support for ref parameter to get raw file API (go-gitea#14602)
  Fixed irritating error message related to go version (go-gitea#14611)
  Use OldRef instead of CommitSHA for DeleteBranch comments (go-gitea#14604)
  Add information on how to build statically (go-gitea#14594)
  [skip ci] Updated translations via Crowdin
  Exclude the current dump file from the dump (go-gitea#14606)
  Remove spurious DataAsync Error logging (go-gitea#14599)
  [API] Add  delete release by tag & fix unreleased inconsistency (go-gitea#14563)
  Fix rate limit bug when downloading assets on migrating from github (go-gitea#14564)
  [API] Add affected files of commits to commit struct (go-gitea#14579)
  [skip ci] Updated licenses and gitignores
  Fix locale init (go-gitea#14582)
  Add Content-Length header to HEAD requests (go-gitea#14542)
  Honor REGISTER_MANUAL_CONFIRM when doing openid registration (go-gitea#14548)
  Fix lfs file viewer (go-gitea#14568)
  Fix typo in generate-emoji.go (go-gitea#14570)
  Fix bug about ListOptions and stars/watchers pagnation (go-gitea#14556)
  Fix gpg key deletion (go-gitea#14561)
  [API] GetRelease by tag only return release (go-gitea#14397)
  Reduce data races (go-gitea#14549)
  ...
@go-gitea go-gitea locked and limited conversation to collaborators Mar 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants