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

Remove unused flags / options #693

Merged
merged 9 commits into from
Jan 31, 2022
Merged

Conversation

anbraten
Copy link
Member

@anbraten anbraten commented Jan 15, 2022

some flags where unused and / or unnecessary as they are covered by alternatives implemented in PRs of milestone 0.15.0 and just complicated the setup.

closes #681

@anbraten anbraten added the enhancement improve existing features label Jan 15, 2022
@6543 6543 added this to the 0.15.0 milestone Jan 17, 2022
@6543
Copy link
Member

6543 commented Jan 29, 2022

I still think we should not drop feature to set custom status context

-> #674

@anbraten anbraten changed the title remove unused flags Remove unused flags / options Jan 29, 2022
@6543
Copy link
Member

6543 commented Jan 30, 2022

normally we dont want the git clone step only to authentificate if needed (repo != public)

but if you set REQUIRE_SIGNIN_VIEW at [service] in gitea (for example - gitlab can be configure similar) you need th auth on "public" repos too (forge api tell us it is public but it is not ...)

I propose: a global bool flag (config) for all forges (similar to context-prefix) to enforce netrc generation

@6543
Copy link
Member

6543 commented Jan 30, 2022

so it's not handled for each remote provider but as a global option for woodpecker itself

@anbraten
Copy link
Member Author

Why shouldn't we always use NETRC credentials to clone repos? We have that data.

@6543
Copy link
Member

6543 commented Jan 30, 2022

because it's sensitive data aka. an secret and if it's not within public repos it has no chance to get leaked at all

@anbraten
Copy link
Member Author

I disagree on that. If passing the secrets is such a high security risk we have that problem for private and public repos and need to think about an alternative for both. As we only pass it to our "verified" clone image at the moment I don't see that it is a security issue.

One thing I want to make clear is the current implementation of PRIVATE_MODE. It is currently not used to decide if we pass NETRC data or not. From how I read the code the only thing it does is setting the woodpecker projects visibility docs. In the case a project is public in the SCM the private mode currently ignores the SCM visibility and sets the woodpecker project visibility to private as well. IMO this isn't really needed. We can simply set the woodpecker projects visibility to the same as the repos visibility of the remote SCM. If a user prefers to change it he is welcome to do so in the UI settings of his wp project.

If we later want to allow the system admin to disallow public access completly, because he only want to allow access to employees for example, we can simply readd a global option, which would change the whole UI & api to always require auth, but that would be a new feature IMO.

@6543
Copy link
Member

6543 commented Jan 31, 2022

hmm well I think the edgecase I mention is also broken in current state - so lets remove things and if it's needed add a new flag for this specific case ...

@6543 6543 enabled auto-merge (squash) January 31, 2022 14:05
@6543 6543 merged commit 6af94d7 into woodpecker-ci:master Jan 31, 2022
6543 pushed a commit that referenced this pull request Feb 8, 2022
As a developer using an custom git server (e.g. Github Enterprise) I would like to be able to authenticate 
the user on repositories which are marked as public.

See issue: #473

Ref: #693 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants