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

Allow separate gitea oauth URL #3513

Merged
merged 11 commits into from
Mar 21, 2024
Merged

Conversation

qwerty287
Copy link
Contributor

@qwerty287 qwerty287 commented Mar 20, 2024

closes #3470

@qwerty287 qwerty287 added forge/gitea gitea forge related enhancement improve existing features labels Mar 20, 2024
@qwerty287 qwerty287 added this to the 2.5.0 milestone Mar 20, 2024
@qwerty287 qwerty287 requested a review from a team March 20, 2024 08:01
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Mar 20, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-3513.surge.sh

@xoxys
Copy link
Member

xoxys commented Mar 20, 2024

IMO, this should also be named _DEV_ to outline that this is not required in most production setup. I would also prefer OAuthURL over OAuth2URL but no strong feelings about this.

closes #3510 seems to be a drive-by fix that might better go in its own PR.

@qwerty287
Copy link
Contributor Author

IMO, this should also be named _DEV_ to outline that this is not required in most production setup.

I don't think so as it's not a dev flag. Maybe we can add some note in docs that most won't need it?

I would also prefer OAuthURL over OAuth2URL but no strong feelings about this.

changed

@xoxys
Copy link
Member

xoxys commented Mar 20, 2024

I don't think so as it's not a dev flag. Maybe we can add some note in docs that most won't need it?

But for what real world scenario this is required? It just applies to dev/test setups, otherwise you have an accessible external domain anyway, or am I wrong?

@qwerty287
Copy link
Contributor Author

Hmm you're right, I just checked the issue again, will change it.

@anbraten
Copy link
Member

anbraten commented Mar 20, 2024

If I didn't missed anything WOODPECKER_DEV_OAUTH_HOST is exactly doing the same already 🤔 . At least I am using it successfully locally with gitea during development.

@xoxys
Copy link
Member

xoxys commented Mar 20, 2024

Yes me too, and I was wondering as well but had no time to check the details yet.

@qwerty287
Copy link
Contributor Author

I don't think so as WOODPECKER_DEV_OAUTH_HOST is about the Woodpecker address/host used for redirection from forge, but the issue is about the Gitea address/host for redirection from WP to Gitea . I also suggested that first in the issue but it didn't solve the problem

@xoxys
Copy link
Member

xoxys commented Mar 20, 2024

Yes, you are right, the WOODPECKER_GITEA_URL needs to be accessible from the container and from the browser. If you can't find a value that fits both environments, this new option is required. Makes sense to me.

server/forge/gitea/gitea.go Outdated Show resolved Hide resolved
server/forge/gitea/gitea_test.go Show resolved Hide resolved
cmd/server/flags.go Outdated Show resolved Hide resolved
Co-authored-by: Robert Kaussow <[email protected]>
Copy link

codecov bot commented Mar 21, 2024

Codecov Report

Attention: Patch coverage is 7.69231% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 36.02%. Comparing base (9c684b7) to head (19adc67).
Report is 1 commits behind head on main.

Files Patch % Lines
cmd/server/setup.go 0.00% 8 Missing ⚠️
server/forge/gitea/gitea.go 20.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3513      +/-   ##
==========================================
- Coverage   36.08%   36.02%   -0.06%     
==========================================
  Files         229      229              
  Lines       15489    15493       +4     
==========================================
- Hits         5589     5582       -7     
- Misses       9494     9504      +10     
- Partials      406      407       +1     

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

@qwerty287 qwerty287 merged commit fbdfa14 into woodpecker-ci:main Mar 21, 2024
7 of 9 checks passed
@qwerty287 qwerty287 deleted the gitea-oauth branch March 21, 2024 10:37
@woodpecker-bot woodpecker-bot mentioned this pull request Mar 20, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing features forge/gitea gitea forge related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a separate WOODPECKER_GITEA_API_URL for easier local setup
4 participants