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

Remove all default privileged plugins #4053

Merged
merged 11 commits into from
Sep 2, 2024
Merged

Remove all default privileged plugins #4053

merged 11 commits into from
Sep 2, 2024

Conversation

6543
Copy link
Member

@6543 6543 commented Aug 25, 2024 •
edited
Loading

re-add plugins to the list via config if needed

@6543
Remove all default privileged plugins
ee4841f 
re-add plugins to the list via config if needed
@6543 6543 added this to the 3.0.0 milestone Aug 25, 2024
@6543 6543 added breaking will break existing installations if no manual action happens security enhancement improve existing features labels Aug 25, 2024
@6543 6543 mentioned this pull request Aug 25, 2024
Merged
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Aug 25, 2024 •
edited
Loading

Tearing down https://woodpecker-ci-woodpecker-pr-4053.surge.sh

@6543 6543 requested a review from anbraten August 25, 2024 20:18
@6543
Copy link
Member Author

6543 commented Aug 25, 2024

I'm personally in fafour of #3918

@6543
Copy link
Member Author

6543 commented Aug 26, 2024

#4056 in combination with #3918 should be good enouth in my poinion ...

@6543 6543 requested review from xoxys and pat-s August 26, 2024 00:30
@codecov Codecov
Copy link

codecov bot commented Aug 26, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 26.94%. Comparing base (5b208d2) to head (1a83e73).

Files with missing lines Patch % Lines
cli/exec/exec.go 0.00% 4 Missing ⚠️
cli/lint/lint.go 0.00% 1 Missing ⚠️
cmd/server/setup.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4053      +/-   ##
==========================================
- Coverage   26.96%   26.94%   -0.03%     
==========================================
  Files         394      394              
  Lines       27416    27418       +2     
==========================================
- Hits         7393     7388       -5     
- Misses      19323    19330       +7     
  Partials      700      700

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pat-s
pat-s reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@pat-s pat-s left a comment

Choose a reason for hiding this comment

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

For the overall issue: I don't see why we should keep only our plugins and I am in favor of removing all from the list.

Besides, similar to my comment in #3918, I think that we are lacking documentation about this change and it should be included in the PR that will be chosen in the end.
As this happened a few times in the past for user-visible changes, I would be really happy to see this being addressed this time.

Last, I think that WOODPECKER_ESCALATED is a strange var name as it doesn't really indicate what "exactly" the setting is doing. Yes, it "escalates" something but that word per se has multiple meanings and is often rather used in a different context.
Why not something more specific like WOODPECKER_ALLOW_PRIVILEGED or WOODPECKER_PLUGINS_PRIVILEGED?

@6543 6543 closed this in #3918 Aug 31, 2024
@6543 6543 reopened this Aug 31, 2024
qwerty287
qwerty287 approved these changes Aug 31, 2024
View reviewed changes
cmd/server/flags.go Outdated Show resolved Hide resolved
6543 added a commit to woodpecker-ci/infrastructure that referenced this pull request Aug 31, 2024
@6543
Add buildx plugin back to priviledged plugins
853bcf7 
will be needed once woodpecker-ci/woodpecker#4053 released merged
@6543 6543 mentioned this pull request Aug 31, 2024
Merged
@6543
Copy link
Member Author

6543 commented Aug 31, 2024

created woodpecker-ci/infrastructure#91

6543
6543 commented Aug 31, 2024
View reviewed changes
cli/exec/flags.go Outdated Show resolved Hide resolved
@6543 6543 merged commit 32d1ec7 into woodpecker-ci:main Sep 2, 2024
6 of 7 checks passed
@6543 6543 deleted the no-privileged-plugins-by-default branch September 2, 2024 08:41
@woodpecker-bot woodpecker-bot mentioned this pull request Sep 2, 2024
Closed
1 task
@6543 6543 mentioned this pull request Sep 4, 2024
Merged
6543 added a commit that referenced this pull request Sep 4, 2024
@6543
Linter: respect tags for linting of global privileged plugins (#4083)
f7d12bf 
https://ci.woodpecker-ci.org/repos/8981/pipeline/162/4 should have shown an lit error but it did not.

this fix it

a followup of #4053
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Sep 4, 2024
@6543
Linter: respect tags for linting of global privileged plugins (woodpe…
6b746e3 
…cker-ci#4083)

https://ci.woodpecker-ci.org/repos/8981/pipeline/162/4 should have shown an lit error but it did not.

this fix it

a followup of woodpecker-ci#4053
@6543 6543 mentioned this pull request Sep 4, 2024
Merged
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
@6543
Remove all default privileged plugins (woodpecker-ci#4053)
43fa583
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
@6543
Linter: respect tags for linting of global privileged plugins (woodpe…
a88231e 
…cker-ci#4083)

https://ci.woodpecker-ci.org/repos/8981/pipeline/162/4 should have shown an lit error but it did not.

this fix it

a followup of woodpecker-ci#4053
@6543 6543 mentioned this pull request Sep 5, 2024
Merged
@woodpecker-bot woodpecker-bot mentioned this pull request Sep 8, 2024
Draft
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anbraten anbraten anbraten left review comments

@pat-s pat-s pat-s left review comments

@qwerty287 qwerty287 qwerty287 approved these changes

@xoxys xoxys Awaiting requested review from xoxys

Assignees
No one assigned
Labels
breaking will break existing installations if no manual action happens enhancement improve existing features security
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@6543 @woodpecker-bot @anbraten @pat-s @qwerty287