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.

Sign up for GitHub

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

Jump to bottom

Show commit status for releases #29149

Merged
merged 4 commits into from
Feb 19, 2024
Merged

Show commit status for releases #29149

merged 4 commits into from
Feb 19, 2024

Conversation

KN4CK3R
Copy link
Member

@KN4CK3R KN4CK3R commented Feb 12, 2024

Fixes #29082

grafik

@KN4CK3R KN4CK3R added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Feb 12, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 12, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 12, 2024
KN4CK3R
KN4CK3R commented Feb 12, 2024
View reviewed changes
templates/repo/release/list.tmpl
@@ -5,90 +5,90 @@
{{template "base/alert" .}}
Copy link
Member Author

Choose a reason for hiding this comment

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

Review this file without whitespace diff to spot the changes.

@yp05327
Copy link
Contributor

yp05327 commented Feb 13, 2024

I have a question. Is it necessary for a stable release.
I can understand add it for a pre-release or a draft release.
But for a stable release, when there's a red x, users may have a question:
is this release really stable? 😕

@delvh
Copy link
Member

delvh commented Feb 13, 2024

And I'd say rightfully so.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 13, 2024

This PR now contains also a possible security fix.

https://try.gitea.io/KN4CK3R/draft-test/releases hides draft versions
https://try.gitea.io/KN4CK3R/draft-test/releases/tag/v1.0 still shows the draft if you can guess the tag name
Don't know if this should get a "bug" or "security" label.

@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 Feb 13, 2024
@a1012112796
Copy link
Member

a little wonder whether this change is necessary enough ? maybe user can add it in the release descraption manually or by ci.

@silverwind
Copy link
Member

maybe user can add it in the release descraption manually or by ci.

Why would one manually add a commit status? It does not make sense imho. If the release can be associated with a tag or commit, we should show the associated commit status, which can indicate problems for example when the release process is done by CI.

@KN4CK3R I think it's possible for release to not be associated to a commit, in such a case we should not render a status.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 16, 2024

@KN4CK3R I think it's possible for release to not be associated to a commit, in such a case we should not render a status.

Releases are tag bases and therefore must be associated to a commit.

@silverwind
Copy link
Member

Okay, I guess it would still be good to add a check so that we do not break the template rendering if no commit can be determined.

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Feb 16, 2024

Where should a check be placed? If there was no commit status found, nothing gets rendered.

@silverwind
Copy link
Member

Before the subtemplate repo/commit_statuses, but if there is a check already, then it's fine. I did not check in detail.

@lunny
Copy link
Member

lunny commented Feb 18, 2024

Maybe it should be hidden if Code is disabled.

kdumontnu
kdumontnu approved these changes Feb 18, 2024
View reviewed changes
Copy link
Contributor

@kdumontnu kdumontnu left a comment

Choose a reason for hiding this comment

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

Looks & works great in my tests 👍

If there is any concern about UI we could optionally move the status next to the commit hash so that it's clear it's related to the underlying ref. I'm happy either way.

@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 Feb 18, 2024
@lunny lunny added this to the 1.22.0 milestone Feb 19, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 19, 2024
@lunny lunny enabled auto-merge (squash) February 19, 2024 09:44
@lunny lunny merged commit 7e8ff70 into go-gitea:main Feb 19, 2024
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 19, 2024
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@KN4CK3R @silverwind
Show commit status for releases (go-gitea#29149)
06e20ae 
Fixes go-gitea#29082

![grafik](https://github.com/go-gitea/gitea/assets/1666336/bb2ccde1-ee99-459d-9e74-0fb8ea79e8b3)
@KN4CK3R KN4CK3R deleted the feature-repo-commit-status branch February 24, 2024 22:53
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@KN4CK3R @earl-warren
Show commit status for releases (go-gitea#29149)
369fe56 
Fixes go-gitea#29082

![grafik](https://github.com/go-gitea/gitea/assets/1666336/bb2ccde1-ee99-459d-9e74-0fb8ea79e8b3)

(cherry picked from commit 7e8ff70)
@lunny lunny mentioned this pull request Feb 28, 2024
Merged
lunny added a commit that referenced this pull request Mar 2, 2024
@lunny
Fix a bug returning 404 when display a single tag with no release (#2…
cc27b50 
…9466)

Partially caused by #29149 

When use

```go
releases, err := getReleaseInfos(ctx, &repo_model.FindReleasesOptions{
		ListOptions: db.ListOptions{Page: 1, PageSize: 1},
		RepoID:      ctx.Repo.Repository.ID,
		TagNames:    []string{ctx.Params("*")},
		// only show draft releases for users who can write, read-only users shouldn't see draft releases.
		IncludeDrafts: writeAccess,
	})
```
replace
```go
release, err := repo_model.GetRelease(ctx, ctx.Repo.Repository.ID, ctx.Params("*"))
```
It missed `IncludeTags: true,`. That means this bug will be occupied only when the release is a tag.
This PR will fix

 - Get the right tag record when it's not a release
 - Display correct tag tab but not release tag when it's a tag.
- The button will bring the tag name to the new page when it's a single tag page
- the new page will automatically hide the release target inputbox when the tag name is pre filled. This should be backport to v1.21.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kdumontnu kdumontnu kdumontnu approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

Actions Status: Release
8 participants
@KN4CK3R @yp05327 @delvh @a1012112796 @silverwind @lunny @kdumontnu @GiteaBot