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

Add scaleway provider #56

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

raskyld
Copy link

@raskyld raskyld commented Nov 11, 2023

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 :)

@raskyld
Copy link
Author

raskyld commented Nov 11, 2023

@anbraten Is it planned to use a config file instead of CLI Flags and Env Vars for users to describe their agent pools?

@qwerty287
Copy link
Contributor

I don't think so because it's using github.com/joho/godotenv which loads env vars from .env files - these act as config file (it's the same for woodpecker itself)

@raskyld
Copy link
Author

raskyld commented Nov 11, 2023

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 godotenv with an environment variable WOODPECKER_AUTOSCALER_AGENT_POOLS_PATH that loads such a file 🤷

@anbraten
Copy link
Member

anbraten commented Nov 12, 2023

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.

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.

@raskyld
Copy link
Author

raskyld commented Nov 12, 2023

Alright, I didn't have the argument that we will use multiple autoscaler per pool, that's a pretty neat design :)

@raskyld
Copy link
Author

raskyld commented Nov 12, 2023

I am going to comply with the CI checks just before the MR leave the draft state btw

@anbraten
Copy link
Member

Alright, I didn't have the argument that we will use multiple autoscaler per pool, that's a pretty neat design :)

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.

@raskyld
Copy link
Author

raskyld commented Nov 12, 2023

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? 🙏

@raskyld
Copy link
Author

raskyld commented Nov 12, 2023

Solves #51

@raskyld raskyld marked this pull request as ready for review November 13, 2023 06:27
@anbraten
Copy link
Member

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? 🙏

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:

@anbraten anbraten changed the title feat: WIP: Scaleway provider Add scaleway provider Nov 13, 2023
providers/scaleway/v1/config.go Outdated Show resolved Hide resolved
providers/scaleway/v1/flags.go Outdated Show resolved Hide resolved
providers/scaleway/v1/flags.go Outdated Show resolved Hide resolved
@raskyld
Copy link
Author

raskyld commented Nov 13, 2023

Thanks for you review, I will try to fix it tonight!

@raskyld
Copy link
Author

raskyld commented Nov 13, 2023

I addressed your concerns, my colleagues gave me one or two tips like using the terminate action instead of deletion to also detach and remove the block storage but I will probably do it in another PR to avoid putting too much in this one :)

@6543
Copy link
Member

6543 commented Nov 21, 2023

could you merge master into the feature branch :)

@raskyld
Copy link
Author

raskyld commented Nov 22, 2023

Done!

@anbraten anbraten added the feature Add new feature label Jan 28, 2024
@xoxys
Copy link
Member

xoxys commented Mar 17, 2024

@raskyld Could you please rebase again?

Signed-off-by: Enzo NOCERA <[email protected]>
Signed-off-by: Enzo NOCERA <[email protected]>
@raskyld
Copy link
Author

raskyld commented Apr 6, 2024

Done :)
Sorry for the delay!

@xoxys
Copy link
Member

xoxys commented Apr 6, 2024

No worries. Thanks!

Comment on lines +29 to +35
case "scaleway":
scwCfg, err := scaleway.FromCLI(ctx)
if err != nil {
return nil, err
}

return scaleway.New(scwCfg, config)
Copy link
Member

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

Copy link
Author

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.

Copy link
Member

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.

providers/scaleway/doc.go Show resolved Hide resolved
@6543 6543 self-requested a review July 18, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Add new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants