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

Cleanup server env settings #3670

Merged
merged 27 commits into from
May 15, 2024
Merged

Conversation

anbraten
Copy link
Member

@anbraten anbraten commented May 1, 2024

ref #3669

Following the sequence from the image (a rather advanced setup, for simple setups most options could be avoided) all possibly needed config values are:

  • red: WOODPECKER_HOST address of woodpecker UI / api (needs to be accessible by browser)
  • blue: WOODPECKER_OAUTH_HOST address of the forge (needs to be accessible by browser)
  • green: WOODPECKER_FORGE_URL address of the forge (needs to be accessible by woodpecker-server)
  • yellow: WOODPECKER_WEBHOOK_HOST address of woodpecker UI / api (needs to be accessible by forge)

Namings of the env variables are not fixed yet and suggestions if they should have a prefix like DEV_ / INTERNAL_ or not are very welcome (we just need to add old namings to not add breaking changes).

Untitled-2024-04-30-0827

@anbraten anbraten changed the title cleanup some settings Cleanup server env settings May 1, 2024
server/forge/gitea/gitea.go Outdated Show resolved Hide resolved
@anbraten anbraten added this to the 2.5.0 milestone May 1, 2024
docs/docs/91-migrations.md Outdated Show resolved Hide resolved
docs/docs/91-migrations.md Outdated Show resolved Hide resolved
@anbraten anbraten marked this pull request as ready for review May 2, 2024 13:47
@anbraten anbraten requested a review from a team May 2, 2024 14:46
@xoxys
Copy link
Member

xoxys commented May 2, 2024

I added a comment about the naming. Did you decided against it or missed it?

@6543
Copy link
Member

6543 commented May 2, 2024

we should update/ehnhance https://woodpecker-ci.org/docs/development/ui ?

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented May 2, 2024

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

@qwerty287 qwerty287 added the refactor delete or replace old code label May 3, 2024
xoxys
xoxys previously approved these changes May 3, 2024
Copy link
Member

@xoxys xoxys left a comment

Choose a reason for hiding this comment

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

LGTM in general, personally I would prefer an INTERNAL_ prefix instead of DEV_.

server/forge/gitea/gitea.go Outdated Show resolved Hide resolved
server/forge/gitea/gitea.go Outdated Show resolved Hide resolved
@zc-devs
Copy link
Contributor

zc-devs commented May 4, 2024

I've drawn a diagram. Feel free to edit.
wp-schema.drawio.txt

@qwerty287
Copy link
Contributor

If we decided to use _INTERNAL_, can you add that to https://woodpecker-ci.org/docs/usage/terminiology#conventions?

cmd/server/server.go Outdated Show resolved Hide resolved
cmd/server/server.go Outdated Show resolved Hide resolved
docs/docs/91-migrations.md Outdated Show resolved Hide resolved
@anbraten
Copy link
Member Author

Should we deprecate WOODPECKER_DEV_WWW_PROXY as well and rename it to WOODPECKER_INTERNAL_WWW_PROXY?

The WOODPECKER_DEV_WWW_PROXY setting only makes sense for developers and therefore seems to be sth different to me compared to the INTERNAL_ settings which can make sense for users, but should normally not be touched.

@anbraten
Copy link
Member Author

anbraten commented May 14, 2024

Now we have WOODPECKER_INTERNAL_FORGE_OAUTH_HOST that still holds the "server fully qualified url. Why is it named _FORGE_OAUTH_HOST then? IMO, that's confusing and should be WOODPECKER_INTERNAL_OAUTH_HOST.

I named it WOODPECKER_INTERNAL_OAUTH_HOST at the beginning as well. However now as qwerty requested it to be an actual forge option I've added the FORGE_ prefix to be consistent will the other forge options.

@xoxys
Copy link
Member

xoxys commented May 15, 2024

With the latest changes, it's completely broken for me:

      WOODPECKER_HOST: http://localhost:8000
      WOODPECKER_GITEA: true
      WOODPECKER_FORGE_URL: http://localhost:3000
      WOODPECKER_INTERNAL_FORGE_OAUTH_HOST: http://woodpecker-gitea:3000
      WOODPECKER_INTERNAL_WEBHOOK_HOST: http://woodpecker-server:8000

After clicking on the login button in Woodpecker, it tries to redirect to http://woodpecker-gitea:3000/login/oauth/authorize now.

@anbraten
Copy link
Member Author

anbraten commented May 15, 2024

After clicking on the login button in Woodpecker, it tries to redirect to http://woodpecker-gitea:3000/login/oauth/authorize now.

That seems to be correct: fully qualified forge url (<scheme>://<host>[/<prefixpath>]). use it if your forge url WOODPECKER_FORGE_URL or WOODPECKER_GITEA_URL, ... isn't a public url.
You probably want it the other way around:

      WOODPECKER_HOST: http://localhost:8000
      WOODPECKER_GITEA: true
      WOODPECKER_FORGE_URL: http://woodpecker-gitea:3000 # forge url used by woodpecker to call forge
      WOODPECKER_INTERNAL_FORGE_OAUTH_HOST: http://localhost:3000 # public forge url
      WOODPECKER_INTERNAL_WEBHOOK_HOST: http://woodpecker-server:8000

@xoxys
Copy link
Member

xoxys commented May 15, 2024

Now I'm fully confused.... WOODPECKER_INTERNAL_FORGE_OAUTH_HOST is named "INTERNAL" that this should now hold the public url while WOODPECKER_INTERNAL_WEBHOOK_HOST holds the internal URL makes no sense to me.

@xoxys
Copy link
Member

xoxys commented May 15, 2024

What's required:

The forge server needs to be reachable from the browser and the wp-server container:

  • WOODPECKER_FORGE_URL should be the one reachable from the browser
  • WOODPECKER_INTERNAL_FORGE_OAUTH_HOST should be the one reachable from container, don't need to be set if WOODPECKER_FORGE_URL can already be resolved/reached from the container as well

Same applies for the webhook part.

@anbraten
Copy link
Member Author

anbraten commented May 15, 2024

INTERNAL was introduced instead of DEV_ #3670 (comment) to let the user now it is a setting that should normally not be touched. It's not about the forge-oauth-host being internal or something like that.

@xoxys
Copy link
Member

xoxys commented May 15, 2024

OK, let me rephrase it. WOODPECKER_FORGE_URL should be the URL that is used by regular users. In most cases, e.g. gitea.example.com. This url can be resolved by the container and browser, everything works as expected.

If we now have an environment where the url needed for browser and container access is not the same, I need to flip it? That's IMO a bad UX.

@xoxys
Copy link
Member

xoxys commented May 15, 2024

Some more tests:

      WOODPECKER_HOST: http://localhost:8000
      WOODPECKER_INTERNAL_WEBHOOK_HOST: http://woodpecker-server:8000
      WOODPECKER_FORGE_URL: http://woodpecker-gitea:3000
      WOODPECKER_INTERNAL_FORGE_OAUTH_HOST: http://localhost:3000
  • Login flow works
  • Trigger CI run by webhook works
  • Clone failed as it tries to clone from http://localhost:3000
fatal: unable to access 'http://localhost:3000/testorg/test.git/': Failed to connect to localhost port 3000 after 0 ms: Couldn't connect to server

      WOODPECKER_HOST: http://localhost:8000
      WOODPECKER_INTERNAL_WEBHOOK_HOST: http://woodpecker-server:8000
      WOODPECKER_FORGE_URL: http://localhost:3000
      WOODPECKER_INTERNAL_FORGE_OAUTH_HOST: http://woodpecker-gitea:3000

Requirements:

  • a way to access the woodpecker-server from browser
  • a way to access the woodpecker-server from forge container
  • a way to access forge-server from browser
  • a way to access forge-server from woodpecker container

Question:

What's the correct configuration?

@anbraten
Copy link
Member Author

anbraten commented May 15, 2024

Clone failed as it tries to clone from http://localhost:3000/

That url is coming from gitea itself. You have to set the correct public url in gitea as well.

Something like: GITEA__server__ROOT_URL: http://woodpecker-gitea:3000
or ROOT_URL: http://woodpecker-gitea:3000 wherever you are using the env variables or the .ini file.

@zc-devs
Copy link
Contributor

zc-devs commented May 15, 2024

INTERNAL was introduced instead of DEV_ #3670 (comment) to let the user now it is a setting that should normally not be touched.

If it is in the docs, then doesn't look "internal" to me anymore (#3670 (comment)).

It's not about the forge-oauth-host being internal or something like that.

Well, I treat it like that. That was the meaning of WOODPECKER_WEBHOOK_HOST.

Seems, it is still confusing...

//localhost

Doesn't look like public faced URL... #3669 (reply in thread)


Draw what you want to achieve, then it can be discussed. It difficult to understand stuff like localhost, woodpecker-server, woodpecker-gitea. My understanding is in #3670 (comment).

Though, requirements section in #3670 (comment) seems right to me.

@xoxys
Copy link
Member

xoxys commented May 15, 2024

Looks like we have (again) a communication issue. It's probably up to me then, however, no idea how to improve it. From my perspective, I have clearly communicated what my problem is and what I would expect from these settings.

That url is coming from gitea itself. You have to set the correct public url in gitea as well.

OK, in that case the env vars seem to work as expected then. The rest of the issue seems to be related to my localhost dev setup, can solve that somehow...

@xoxys xoxys dismissed their stale review May 15, 2024 10:43

seems to work - kind of

@anbraten
Copy link
Member Author

anbraten commented May 15, 2024

As other users might be confused by the INTERNAL_ prefix as well, I've renamed them to EXPERT_, so users will hopefully know that they are only required in complex setups and will normally be avoided.

@anbraten anbraten requested a review from xoxys May 15, 2024 11:08
@xoxys
Copy link
Member

xoxys commented May 15, 2024

IMO, EXPERT_ sound a bit weird. I would keep INTERNAL_ and just extend the documentation if required. However, no strong feelings.

@anbraten anbraten merged commit 5527d9b into woodpecker-ci:main May 15, 2024
6 of 7 checks passed
@woodpecker-bot woodpecker-bot mentioned this pull request May 14, 2024
1 task
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
Co-authored-by: Robert Kaussow <[email protected]>
Co-authored-by: Robert Kaussow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor delete or replace old code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants