-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add scaleway provider #56
base: main
Are you sure you want to change the base?
Conversation
@anbraten Is it planned to use a config file instead of CLI Flags and Env Vars for users to describe their agent pools? |
I don't think so because it's using github.com/joho/godotenv which loads env vars from |
We should discuss about it because in terms of UX, if we want the auto-scaler to manage different agent pools (with different labels set) this will be really heavy on environment variables and a structured file format like YAML or TOML would be better IMHO. As a middle-ground maybe we could use |
Currently the design is to use one autoscaler per agent-pool, so you would just need to configure one filter per autoscaler. Its a pretty lightweight tool and makes the code / config much simpler for now. In container / deployment setups env vars are normally way easier to setup IMO than mounting a config file, so wouldn't per se agree that a config file is better suited / UX. |
Alright, I didn't have the argument that we will use multiple autoscaler per pool, that's a pretty neat design :) |
I am going to comply with the CI checks just before the MR leave the draft state btw |
It might be a consideration in the long-run to not have 100 (lets first get to such an amount 😅) agents querying the server for queue information, but til then I think it ways easier to have separate ones. |
I am going to test the PR later in the afternoon, do you have any hint on how to setup a testing environment as I am pretty new to WP? 🙏 |
Solves #51 |
I normally test using a simple docker-compose with gitea. Sth like this: version: '3'
services:
woodpecker-server:
image: woodpeckerci/woodpecker-server:next
ports:
- 8000:8000
volumes:
environment:
- WOODPECKER_OPEN=true
- WOODPECKER_ADMIN=anbraten
- WOODPECKER_HOST=${WOODPECKER_HOST}
- WOODPECKER_GITEA=true
- WOODPECKER_GITEA_CLIENT=${WOODPECKER_GITHUB_CLIENT}
- WOODPECKER_GITEA_SECRET=${WOODPECKER_GITHUB_SECRET}
gitea-database:
image: postgres:12.4-alpine
environment:
POSTGRES_USER: gitea
POSTGRES_PASSWORD: 123456
POSTGRES_DB: gitea
PGDATA: /var/lib/postgresql/data/pgdata
volumes:
- pgsql:/var/lib/postgresql/data/pgdata
gitea:
image: gitea/gitea:1.21
ports:
- 3000:3000
volumes:
- gitea:/data
- /etc/timezone:/etc/timezone:ro
- /etc/localtime:/etc/localtime:ro
depends_on:
- gitea-database
environment:
USER_UID: 1000
USER_GID: 1000
# GITEA__server__DOMAIN: gitea.local.self
GITEA__server__ROOT_URL: http://gitea:3000
GITEA__database__DB_TYPE: postgres
GITEA__database__HOST: gitea-database:5432
GITEA__database__NAME: gitea
GITEA__database__USER: gitea
GITEA__database__PASSWD: 123456
GITEA__webhook__ALLOWED_HOST_LIST: '*'
extra_hosts:
- 'host.docker.internal:host-gateway'
volumes:
gitea:
pgsql:
woodpecker-server-data: |
Thanks for you review, I will try to fix it tonight! |
I addressed your concerns, my colleagues gave me one or two tips like using the |
could you merge master into the feature branch :) |
Done! |
@raskyld Could you please rebase again? |
65294e1
to
13739b0
Compare
- chore(doc): cleaning - chore(ci): run gofumpt Signed-off-by: Enzo NOCERA <[email protected]>
Signed-off-by: Enzo NOCERA <[email protected]>
Signed-off-by: Enzo NOCERA <[email protected]>
Signed-off-by: Enzo NOCERA <[email protected]>
Signed-off-by: Enzo NOCERA <[email protected]>
Signed-off-by: Enzo NOCERA <[email protected]>
13739b0
to
a9b3feb
Compare
Signed-off-by: Enzo NOCERA <[email protected]>
Done :) |
No worries. Thanks! |
case "scaleway": | ||
scwCfg, err := scaleway.FromCLI(ctx) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
return scaleway.New(scwCfg, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a specific reason to use another structure to do the config setup for this provider? I would prefer to keep the setup for providers equal. Everything done by scaleway.FromCLI(ctx)
should be handled by scaleway.New
similar to e.g. https://github.com/woodpecker-ci/autoscaler/blob/main/providers/hetznercloud/provider.go#L43
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I initially decoupled the parsing from CLI and the config structure to allow using declarative file (e.g. a yaml to config the autoscaler) instead of the CLI. But it seems like we won't support configuration from file so I can remove that indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have some time to change it that would be great. If not we can also merge the current state and change it later.
I've started to draft a Scaleway provider, the first commit contains only the API and the links between the CLI flags and the said API.
I open it so you can start suggestions / remarks :)