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

Hiding Secrets options when Actions feature is disabled #24792

Merged
merged 1 commit into from
May 24, 2023

Conversation

pboguslawski
Copy link
Contributor

Secrets options should be hidden if Actions feature is disabled.

This fixes in release/v1.19. In main probably fixed in 63a401a (didn't check).

Fixes: 6590551
Author-Change-Id: IB#1134011

`Secrets` options should be hidden if `Actions` feature is disabled.

Fixes: 6590551
Author-Change-Id: IB#1134011
Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>
@GiteaBot GiteaBot added this to the 1.19.4 milestone May 18, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 18, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 18, 2023
@delvh
Copy link
Member

delvh commented May 18, 2023

As far as I know, there are plans to use secrets for more than just Actions, so I'm rather against this change.

@delvh
Copy link
Member

delvh commented May 18, 2023

(And no, it's not "fixed" on main as it is not supposed to be "fixed")
Okay, I retract my previous statement.
That caught me by surprise that the commit apparently does something unexpected.

@delvh
Copy link
Member

delvh commented May 18, 2023

Nevertheless, enhancements are typically not backported once x.0 is released.
So I'm still rather against this change.

@pboguslawski
Copy link
Contributor Author

there are plans to use secrets for more than just Actions

Could you elaborate?

If optional, should be equipped with option to disable it.

@pboguslawski
Copy link
Contributor Author

Nevertheless, enhancements are typically not backported once x.0 is released.

It's not enhancement but bugfix IHMO (assumes that Secrets are used only for Actions in 1.19).

@delvh
Copy link
Member

delvh commented May 18, 2023

At the moment, secrets are only used for Actions.
However, I've already heard of plans to allow them for other things as well, i.e. to send them inside webhooks, for the package registry or for external CIs.
If I remember correctly, it was @lunny who expressed something in that direction.


Regarding bugfix:
I disagree that it's a bug.
A bug is an existing feature that somehow misbehaves for Gitea (i.e. someone can see a secret they are not supposed to see).
An enhancement on the other hand is the addition of new code for an existing feature that doesn't misbehave but whose behavior can be improved.

The fact that you can only use the secrets for Actions at the moment doesn't mean that secrets should be disabled if Actions are disabled.
If you set up your secrets now already, you can still use them in additional ways once possible.
I do agree that a setting would probably be the best course of action.

@pboguslawski
Copy link
Contributor Author

However, I've already heard of plans to allow them for other things as well, i.e. to send them inside webhooks, for the package registry or for external CIs.

So Secrets should be enabled in 1.19 only when Actions are enabled (like in this PR) and when other optional features like webhooks require Secrets in future, Secrets should only be displayed when any of features requiring it is enabled (and hidden otherwise to not confuse users like now).

@wolfogre
Copy link
Member

At the moment, secrets are only used for Actions. However, I've already heard of plans to allow them for other things as well, i.e. to send them inside webhooks, for the package registry or for external CIs. If I remember correctly, it was @lunny who expressed something in that direction.

Just my opinions:

  • Admit that the secrets Gitea has now are only used for Actions, it should be disabled when Actions has been disabled.
  • One day secrets may be used for something else, but don't mix them.
    • Add a type for secrets, like actions and webhook
    • Different types of secrets have different UI entry points.
    • Actions will use secrets with actions type only.
  • Then the UI entry point for secrets for Actions should be disabled when Actions have been disabled, but others could be enabled.

So I agree with the idea of PR, but I also agree with delvh's opinion: it's not a bug. I don't think it has to be fixed in 1.19.

@lunny
Copy link
Member

lunny commented May 19, 2023

Secrets should be split into serval groups and different groups should not share the secrets. So the current secrets under actions should be the part used only by Actions.

@silverwind
Copy link
Member

silverwind commented May 19, 2023

Secrets should be split into serval groups and different groups should not share the secrets. So the current secrets under actions should be the part used only by Actions.

That does sound like confusing UI, I think there should only be one set of secrets per repo/org and a global set for the whole instance, managed in the admin ui.

@lunny
Copy link
Member

lunny commented May 19, 2023

Secrets should be split into serval groups and different groups should not share the secrets. So the current secrets under actions should be the part used only by Actions.

That does sound like confusing UI, I think there should only be one set of secrets per repo/org and a global set for the whole instance, managed in the admin ui.

Since currently all secrets are for Actions, I think we can think it's all for Actions. Once we have another PR that supports to use secrets on other places, then it's the responsibility of that PR to do the UI change.

@silverwind
Copy link
Member

silverwind commented May 19, 2023

Secrets should be split into serval groups and different groups should not share the secrets. So the current secrets under actions should be the part used only by Actions.

That does sound like confusing UI, I think there should only be one set of secrets per repo/org and a global set for the whole instance, managed in the admin ui.

Since currently all secrets are for Actions, I think we can think it's all for Actions. Once we have another PR that supports to use secrets on other places, then it's the responsibility of that PR to do the UI change.

Visually, I'd follow GitHub, e.g.

Repo > Secrets > Actions (per-repo)
Org > Secrets > Actions (per-org)
Admin > Secrets > Actions (global)

in case of name conflict, more specific secrets win, e.g. repo > org > global.

@lunny
Copy link
Member

lunny commented May 23, 2023

Secrets should be split into serval groups and different groups should not share the secrets. So the current secrets under actions should be the part used only by Actions.

That does sound like confusing UI, I think there should only be one set of secrets per repo/org and a global set for the whole instance, managed in the admin ui.

Since currently all secrets are for Actions, I think we can think it's all for Actions. Once we have another PR that supports to use secrets on other places, then it's the responsibility of that PR to do the UI change.

Visually, I'd follow GitHub, e.g.

Repo > Secrets > Actions (per-repo) Org > Secrets > Actions (per-org) Admin > Secrets > Actions (global)

in case of name conflict, more specific secrets win, e.g. repo > org > global.

I think it's OK. Let's close this one.
@pboguslawski what do you think about this?

@pboguslawski
Copy link
Contributor Author

@pboguslawski what do you think about this?

You decided that secrets should be part of Actions in 1.19 if understood correctly but now are not (Secrets visible even if actions are disabled). This may confuse users.

@lunny
Copy link
Member

lunny commented May 23, 2023

Current Secrets are only for Actions, in the future, we can have more options, like what Github did.

图片

So I can accept both changes for now.

  • Solution 1: Since no more other type secrets currently, move secrets under Actions and hide all of them when actions disabled.
  • Solution 2: Move secrets out of Actions as a standalone tab and creating a sub type named Actions like what Github did.

@pboguslawski
Copy link
Contributor Author

Proposed changes are enhancements (problably interfering with 63a401a) and this PR is quick bugfix for 1.19 only. Feel free to close it if you don't like fixing it in 1.19.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

For 1.19 only , I think it's fine.

@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 May 23, 2023
@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 May 23, 2023
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 23, 2023
@techknowlogick techknowlogick enabled auto-merge (squash) May 23, 2023 17:23
@lunny lunny disabled auto-merge May 24, 2023 10:01
@lunny lunny merged commit c5dee88 into go-gitea:release/v1.19 May 24, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 24, 2023
Codeberg-org pushed a commit to Codeberg-org/gitea that referenced this pull request Jun 3, 2023
`Secrets` options should be hidden if `Actions` feature is disabled.

This fixes in release/v1.19. In main probably fixed in
63a401a (didn't check).

Fixes: 6590551
Author-Change-Id: IB#1134011

Signed-off-by: Pawel Boguslawski <pawel.boguslawski@ib.pl>
(cherry picked from commit c5dee88)
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 22, 2023
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. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants