woodpecker icon indicating copy to clipboard operation
woodpecker copied to clipboard

Add PipelineListsOptions to woodpecker-go

Open xoxys opened this issue 10 months ago • 14 comments

I fear that's a breaking change... It's a bit pain IMO that we can not version the go lib independent of the server.

I don't see a (good) way to make it non-breaking. If you have a better idea, please let me know. However, as this will be breaking anyway, we should replace all other raw query parameters used in the lib (e.g. https://github.com/woodpecker-ci/woodpecker/blob/main/woodpecker-go/woodpecker/repo.go#L6) by proper opt structs as well as this makes it easier to extend with future options.

Changes:

  • Add PipelineListOptions and expose them to cli
❯ ./dist/woodpecker-cli pipeline ls --format "#{{ .Number }} {{ .Started }}" --before "2023-08-21T21:11:02+02:00" 2
#1 1692644861
❯ ./dist/woodpecker-cli pipeline ls --format "#{{ .Number }} {{ .Started }}" --after "2023-12-11T15:07:04+01:00" 2
#43 1702762952

  • Add RepoListOptions and expose them to cli
❯ ./dist/woodpecker-cli repo ls --all
testorg/test (id: 2, forgeRemoteID: 3, isActive: true)
testorg2/test2 (id: 4, forgeRemoteID: 4, isActive: true)
woodpecker/woodpecker-test (id: 0, forgeRemoteID: 1, isActive: false)
testorg/woodpecker-test (id: 6, forgeRemoteID: 2, isActive: true)
  • Add DeployOptions
  • Add PipelineStartOptions
  • Add PipelineLastOptions
  • Add RepoPostOptions
  • Add RepoMoveOptions

xoxys avatar Apr 26 '24 19:04 xoxys

How do we handle such PRs in general? Keep it open until we want to do a major version bump?

xoxys avatar Apr 26 '24 19:04 xoxys

We actually just merged them and ignored that they're breaking...

qwerty287 avatar Apr 27 '24 08:04 qwerty287

Maybe we should just move the lib to a dedicated repo? But same argumentation might apply to cli, agent etc. which would end in a repo mess instead of the current monorepo approach. So maybe a bad idea 🙃

xoxys avatar Apr 27 '24 08:04 xoxys

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

woodpecker-bot avatar Apr 27 '24 20:04 woodpecker-bot

That's correct. It's already on my to-do list thanks for the reminder :)

xoxys avatar Jun 20 '24 07:06 xoxys

before we merge this, I want to have a bugfix release!

6543 avatar Jun 20 '24 16:06 6543

... Nobody wanted to rush through especially not because the PR is not finished and requires a major version bump...

xoxys avatar Jun 20 '24 16:06 xoxys

I agree, but which bugfixes should the contained in this patch release? There's no open/merged PR currently (except some docs enhancements) and AFAIK there was nothing critical reported

qwerty287 avatar Jun 20 '24 17:06 qwerty287

I want a majour bump soon-isch to get some breaking stuff done ...

... so we can just wait till that

6543 avatar Jul 13 '24 21:07 6543

I dont get your point. This PR was marked as breaking all the time. Its was already planned to wait with a merge till the next breaking release all the time.

First you want a bugfix release first (I still dont know how this is related to this PR) now you want a major soonish 🤷‍♂️

Im (again) pretty confused about this communication style. Just dropping a single sentence and not responding to any question is pretty bad.

xoxys avatar Jul 13 '24 21:07 xoxys

Current code lgtm, but you introduced a ListOptions type, but did not apply it to all list api methods.

Missing in: UserList RegistryList CronList SecretList

@qwerty287 Was looking into it again. These client methods don't have any additional options. Should we still add an empty options type?

xoxys avatar Aug 03 '24 20:08 xoxys

If we want to add empty list option types to every list method, what about:

	OrgRegistryList(orgID int64) ([]*Registry, error)
	GlobalRegistryList() ([]*Registry, error)
	OrgSecretList(orgID int64) ([]*Secret, error)
	GlobalSecretList() ([]*Secret, error)
	AgentList() ([]*Agent, error)
	AgentTasksList(int64) ([]*Task, error)

xoxys avatar Aug 03 '24 20:08 xoxys

My comment was about the pagination options. They're not supported yet, you need to add them and also add the query params to the URL.

qwerty287 avatar Aug 04 '24 05:08 qwerty287

ci faild related

6543 avatar Sep 05 '24 22:09 6543