Added Description Field for Secrets and Variables #33526

LaoQi · 2025-02-06T17:59:51Z

models/actions/variable.go

lafriks · 2025-02-07T18:33:11Z

templates/shared/variables/variable_list.tmpl

@@ -77,6 +81,15 @@
 					placeholder="{{ctx.Locale.Tr "secrets.creation.name_placeholder"}}"
 				>
 			</div>
+			<div class="field">
+				<label for="dialog-variable-description">{{ctx.Locale.Tr "secrets.creation.description"}}</label>
+				<input


Should probably also add max length to this field

LaoQi · 2025-02-08T03:13:44Z

Is there a standardized length limit for these texts in the project? Maybe we should define constants for these. For now, I've capped them all at 4096 characters

lunny · 2025-02-09T03:26:39Z

LGTM except #33526 (comment)

Screenshots are necessary.

LaoQi · 2025-02-09T06:56:59Z

I noticed that the data field also doesn't have a length restriction. I'll add a length constraint to it as well. For this field, I think a limit of 65536 would be appropriate, although we can truncate the display when showing it.
The input data is generally UTF-8 encoded, so I will use UTF-8 character counting.
eg:

const (
	VariableDataMaxLength        = 65536
	VariableDescriptionMaxLength = 4096
)
if utf8.RuneCountInString(data) > VariableDataMaxLength {
	data = string([]rune(data)[:VariableDataMaxLength])
}

if utf8.RuneCountInString(description) > VariableDescriptionMaxLength {
	description = string([]rune(description)[:VariableDescriptionMaxLength])
}

Any other suggestions?

LaoQi · 2025-02-09T19:43:55Z

…3484)

lunny · 2025-02-09T21:25:47Z

I think the description should be moved under value because it's optional. And it's better to have 2 rows at least.

LaoQi · 2025-02-10T03:37:12Z

I think the description should be moved under value because it's optional. And it's better to have 2 rows at least.

Does it refer only to the form, or does it also include the list? Perhaps it would be better to place the description in the list above.

LaoQi · 2025-02-10T14:51:45Z

lunny · 2025-02-10T19:19:14Z

models/actions/variable.go

 	if ownerID != 0 && repoID != 0 {
 		// It's trying to create a variable that belongs to a repository, but OwnerID has been set accidentally.
 		// Remove OwnerID to avoid confusion; it's not worth returning an error here.
 		ownerID = 0
 	}

+	if utf8.RuneCountInString(data) > VariableDataMaxLength {


I think we should return failure here because data should not be truncated.

yes, you are right

lunny · 2025-02-10T19:20:10Z

models/actions/variable.go

+		data = string([]rune(data)[:VariableDataMaxLength])
+	}
+
+	if utf8.RuneCountInString(description) > VariableDescriptionMaxLength {


Use util.SplitStringAtByteN directly.

lunny · 2025-02-10T19:20:48Z

models/secret/secret.go

@@ -67,15 +74,25 @@ func InsertEncryptedSecret(ctx context.Context, ownerID, repoID int64, name, dat
 		return nil, fmt.Errorf("%w: ownerID and repoID cannot be both zero, global secrets are not supported", util.ErrInvalidArgument)
 	}

+	if len(data) > SecretDataMaxLength {


I think we should return failure here because data should not be truncated.

lunny · 2025-02-10T19:21:02Z

models/secret/secret.go

+		data = data[:SecretDataMaxLength]
+	}
+
+	if utf8.RuneCountInString(description) > SecretDescriptionMaxLength {


Use util.SplitStringAtByteN directly.

lunny · 2025-02-10T19:22:13Z

templates/shared/secrets/add_list.tmpl

@@ -62,7 +65,6 @@
 				<input autofocus required
 					id="secret-name"
 					name="name"
-					value="{{.name}}"


Why remove this?

Is this property actually needed? I don't see it used anywhere, and the template isn't setting its value.

lunny · 2025-02-10T19:22:33Z

templates/shared/variables/variable_list.tmpl

@@ -72,7 +77,6 @@
 				<input autofocus required
 					name="name"
 					id="dialog-variable-name"
-					value="{{.name}}"


silverwind · 2025-02-14T00:39:23Z

templates/shared/variables/variable_list.tmpl

@@ -24,6 +25,9 @@
 				<div class="flex-item-title">
 					{{.Name}}
 				</div>
+				<div class="flex-item-body">
+					{{if .Description}} {{.Description}} {{else}} - {{end}}


Suggested change

{{if .Description}} {{.Description}} {{else}} - {{end}}

{{if .Description}}{{.Description}}{{else}}-{{end}}

Whitespace inside HTML tags is irrelevant in flexbox. If you really need it for spacing, use   or add margin classes.

silverwind · 2025-02-14T00:41:04Z

templates/shared/secrets/add_list.tmpl

@@ -22,6 +22,9 @@
 				<div class="flex-item-title">
 					{{.Name}}
 				</div>
+				<div class="flex-item-body">
+					{{if .Description}} {{.Description}} {{else}} - {{end}}


Suggested change

{{if .Description}} {{.Description}} {{else}} - {{end}}

{{if .Description}}{{.Description}}{{else}}-{{end}}

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 6, 2025

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 6, 2025

github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/migrations labels Feb 6, 2025

lunny added this to the 1.24.0 milestone Feb 6, 2025

lunny reviewed Feb 7, 2025

View reviewed changes

models/actions/variable.go Show resolved Hide resolved

lafriks reviewed Feb 7, 2025

View reviewed changes

LaoQi force-pushed the main branch from da88bf7 to ad09662 Compare February 9, 2025 19:33

LaoQi added 3 commits February 10, 2025 03:49

Basic support description field for secrets and variables (go-gitea#3…

6735dcd

…3484)

Update swagger definitions for variables and secrets

ce38b10

Add length limits for description and data

068a048

LaoQi force-pushed the main branch from ad09662 to 068a048 Compare February 9, 2025 19:50

LaoQi added 2 commits February 10, 2025 22:48

Swap the positions of Data and Description

16238e0

Merge remote-tracking branch 'base/main'

317ab1d

lunny reviewed Feb 10, 2025

View reviewed changes

silverwind reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Description Field for Secrets and Variables #33526

Added Description Field for Secrets and Variables #33526

LaoQi commented Feb 6, 2025

lafriks Feb 7, 2025

LaoQi commented Feb 8, 2025

lunny commented Feb 9, 2025 •

edited

Loading

LaoQi commented Feb 9, 2025

LaoQi commented Feb 9, 2025

lunny commented Feb 9, 2025

LaoQi commented Feb 10, 2025

LaoQi commented Feb 10, 2025

lunny Feb 10, 2025 •

edited

Loading

LaoQi Feb 11, 2025

lunny Feb 10, 2025

lunny Feb 10, 2025

lunny Feb 10, 2025

lunny Feb 10, 2025

LaoQi Feb 11, 2025

lunny Feb 10, 2025

silverwind Feb 14, 2025 •

edited

Loading

silverwind Feb 14, 2025

	{{if .Description}} {{.Description}} {{else}} - {{end}}
	{{if .Description}}{{.Description}}{{else}}-{{end}}

Added Description Field for Secrets and Variables #33526

Are you sure you want to change the base?

Added Description Field for Secrets and Variables #33526

Conversation

LaoQi commented Feb 6, 2025

Choose a reason for hiding this comment

LaoQi commented Feb 8, 2025

lunny commented Feb 9, 2025 • edited Loading

LaoQi commented Feb 9, 2025

LaoQi commented Feb 9, 2025

lunny commented Feb 9, 2025

LaoQi commented Feb 10, 2025

LaoQi commented Feb 10, 2025

lunny Feb 10, 2025 • 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

silverwind Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Feb 9, 2025 •

edited

Loading

lunny Feb 10, 2025 •

edited

Loading

silverwind Feb 14, 2025 •

edited

Loading