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

Actions are no longer experimental, so enable them by default #27054

Merged
merged 5 commits into from
Sep 15, 2023
Merged

Actions are no longer experimental, so enable them by default #27054

merged 5 commits into from
Sep 15, 2023

Conversation

lunny
Copy link
Member

@lunny lunny commented Sep 13, 2023

This PR makes the actions enabled by default, so people will find it easier to enable actions in repository setting.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 13, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 13, 2023
@lunny lunny added this to the 1.21.0 milestone Sep 13, 2023
@JakobDev
Copy link
Contributor

I'm not sure if this should be enabled by default. You still need a runner, so this will be not working by default.

@lunny
Copy link
Member Author

lunny commented Sep 13, 2023

I'm not sure if this should be enabled by default. You still need a runner, so this will be not working by default.

Yes, but this change will reduce people asking why they cannot get the actions feature.

@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 Sep 13, 2023
@techknowlogick
Copy link
Member

Should we add it as a default unit to repo_units too?

@jolheiser
Copy link
Member

jolheiser commented Sep 13, 2023 •
edited
Loading

I'm also not convinced it should be enabled by default, considering it takes additional setup to get it working.
What about instead adding it to the install page? It's getting a little big, but overall I think that might make more sense.

Yes, but this change will reduce people asking why they cannot get the actions feature.

I think it may instead increase the amount of "why aren't actions working" questions if they are enabled but have no runners by default.

@techknowlogick
Copy link
Member

I think it may instead increase the amount of "why aren't actions working" questions if they are enabled but have no runners by default.

In that case we could add a warning to repo owners with actions pending that they need to add runners?

@jolheiser
Copy link
Member

I think it may instead increase the amount of "why aren't actions working" questions if they are enabled but have no runners by default.

In that case we could add a warning to repo owners with actions pending that they need to add runners?

I think that may be a worthwhile change in itself, but it seems like of the two main install types:

  1. Users who use the install page, a checkbox for enabling actions along with a link to "how to set up runners" may be a good solution, or
  2. Users who deploy with a ready config should likely be reading and understanding the configuration for actions/runners. Anyone in this category would also already have them enabled and so unaffected by this PR, but anyone who doesn't have them enabled will likely want to review before it is turned on for them.

It just seems like flipping the toggle from false to true doesn't improve the situation as a default, it just moves the work elsewhere.

As another alternative, a db-setting may work fine here as well, so it becomes easier to just go flip the switch when the admin is ready.

@delvh delvh changed the title Convert actions from expeirement to production ready, so make it enabled default Actions are no longer experimental, so enable them by default Sep 13, 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 Sep 13, 2023
delvh
delvh previously requested changes Sep 13, 2023
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

Please also update the config cheat sheet and the example config.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. labels Sep 13, 2023
@silverwind
Copy link
Member

silverwind commented Sep 13, 2023 •
edited
Loading

And update app.example. ini as usual.

I guess I don't mind it enabled by default if it does nothing. What we should eventually do is enable it by default in repos. Of course it should only run if a workflow file is detected and at least 1 (recently active) worker is registered.

@lunny
Copy link
Member Author

lunny commented Sep 14, 2023

Should we add it as a default unit to repo_units too?

I think that may have a bigger impact for users which can be leave to other PRs. This PR will not affect anything, the actions tab will not be enabled except they enable it in repository setting.

@github-actions github-actions bot added the type/docs This PR mainly updates/creates documentation label Sep 14, 2023
@lunny lunny dismissed delvh’s stale review September 14, 2023 02:59

config cheat sheet and the example config updated

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Sep 14, 2023
@lunny
Copy link
Member Author

lunny commented Sep 14, 2023

I'm also not convinced it should be enabled by default, considering it takes additional setup to get it working. What about instead adding it to the install page? It's getting a little big, but overall I think that might make more sense.

Yes, but this change will reduce people asking why they cannot get the actions feature.

I think it may instead increase the amount of "why aren't actions working" questions if they are enabled but have no runners by default.

This global configuration will not enable repository actions by default. In fact, after this PR, every repository still needs to enable actions in the setting.

@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 Sep 15, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 15, 2023
@lunny lunny enabled auto-merge (squash) September 15, 2023 06:13
@lunny lunny merged commit e5ec57c into go-gitea:main Sep 15, 2023
25 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 15, 2023
@lunny lunny deleted the lunny/enable_actions_global_default branch September 15, 2023 09:33
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 17, 2023
@zjjhot
Merge remote-tracking branch 'giteaoffical/main'
1434f7a 
* giteaoffical/main: (23 commits)
  Search branches (go-gitea#27055)
  Fix wrong migration for email address (go-gitea#27106)
  [skip ci] Updated translations via Crowdin
  Support `.git-blame-ignore-revs` file (go-gitea#26395)
  Add `RemoteAddress` to mirrors (go-gitea#26952)
  Upgrading the actions/checkout@4 (go-gitea#27096)
  Next round of `db.DefaultContext` refactor (go-gitea#27089)
  Ui correction in mobile view nav bar left aligned items. (go-gitea#27046)
  Add missing deps to files-changed (go-gitea#27100)
  Use db.WithTx for AddTeamMember to avoid ctx abuse (go-gitea#27095)
  Drop Node.js 16 and update js dependencies (go-gitea#27094)
  Fix NPE when editing OAuth2 applications (go-gitea#27078)
  Use `print` instead of `printf` (go-gitea#27093)
  Add tests for db indexer in indexer_test.go (go-gitea#27087)
  [skip ci] Updated translations via Crowdin
  Allow empty Conan files (go-gitea#27092)
  Actions are no longer experimental, so enable them by default (go-gitea#27054)
  Update brew installation documentation since gitea moved to brew core package (go-gitea#27070)
  More refactoring of `db.DefaultContext` (go-gitea#27083)
  [skip ci] Updated translations via Crowdin
  ...
silverwind added a commit to silverwind/gitea that referenced this pull request Sep 17, 2023
@silverwind
Merge remote-tracking branch 'origin/main' into usefetch
8a6e43c 
* origin/main: (53 commits)
  Search branches (go-gitea#27055)
  Fix wrong migration for email address (go-gitea#27106)
  [skip ci] Updated translations via Crowdin
  Support `.git-blame-ignore-revs` file (go-gitea#26395)
  Add `RemoteAddress` to mirrors (go-gitea#26952)
  Upgrading the actions/checkout@4 (go-gitea#27096)
  Next round of `db.DefaultContext` refactor (go-gitea#27089)
  Ui correction in mobile view nav bar left aligned items. (go-gitea#27046)
  Add missing deps to files-changed (go-gitea#27100)
  Use db.WithTx for AddTeamMember to avoid ctx abuse (go-gitea#27095)
  Drop Node.js 16 and update js dependencies (go-gitea#27094)
  Fix NPE when editing OAuth2 applications (go-gitea#27078)
  Use `print` instead of `printf` (go-gitea#27093)
  Add tests for db indexer in indexer_test.go (go-gitea#27087)
  [skip ci] Updated translations via Crowdin
  Allow empty Conan files (go-gitea#27092)
  Actions are no longer experimental, so enable them by default (go-gitea#27054)
  Update brew installation documentation since gitea moved to brew core package (go-gitea#27070)
  More refactoring of `db.DefaultContext` (go-gitea#27083)
  [skip ci] Updated translations via Crowdin
  ...
@lunny lunny mentioned this pull request Sep 20, 2023
Closed
@yp05327 yp05327 mentioned this pull request Nov 22, 2023
Merged
lunny pushed a commit that referenced this pull request Nov 22, 2023
@yp05327
Fix the description about the default setting for action in quick sta…
2d9b91a 
…rt document (#28160)

Since #27054, Actions are enabled by default. so we should also edit the
document. 😃

ps: I think this should be backport to 1.21.0.
@GiteaBot GiteaBot mentioned this pull request Nov 22, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Nov 22, 2023
@yp05327 @GiteaBot
Fix the description about the default setting for action in quick sta…
2f2896c 
…rt document (go-gitea#28160)

Since go-gitea#27054, Actions are enabled by default. so we should also edit the
document. 😃

ps: I think this should be backport to 1.21.0.
lunny pushed a commit that referenced this pull request Nov 22, 2023
@GiteaBot @yp05327
Fix the description about the default setting for action in quick sta…
447422f 
…rt document (#28160) (#28168)

Backport #28160 by @yp05327

Since #27054, Actions are enabled by default. so we should also edit the
document. 😃

ps: I think this should be backport to 1.21.0.

Co-authored-by: yp05327 <576951401@qq.com>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@denyskon denyskon denyskon approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@delvh delvh delvh left review comments

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/docs This PR mainly updates/creates documentation type/miscellaneous
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.
9 participants
@lunny @JakobDev @techknowlogick @jolheiser @silverwind @wxiaoguang @denyskon @delvh @GiteaBot