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 PipelineListsOptions to woodpecker-go #3652

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

Add PipelineListsOptions to woodpecker-go #3652

wants to merge 13 commits into from

Conversation

xoxys
Copy link
Member

@xoxys xoxys commented Apr 26, 2024

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 xoxys requested a review from a team April 26, 2024 19:33
@xoxys xoxys added breaking will break existing installations if no manual action happens lib about our woodpecker-go api lib labels Apr 26, 2024
@xoxys
Copy link
Member Author

xoxys commented Apr 26, 2024

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

@qwerty287
Copy link
Contributor

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

@xoxys
Copy link
Member Author

xoxys commented Apr 27, 2024

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 🙃

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Apr 27, 2024

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

Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

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

Thanks for adding tests

@xoxys
Copy link
Member Author

xoxys commented Jun 20, 2024

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

@6543
Copy link
Member

6543 commented Jun 20, 2024

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

@xoxys
Copy link
Member Author

xoxys commented Jun 20, 2024

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

@qwerty287
Copy link
Contributor

qwerty287 commented Jun 20, 2024

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

@6543
Copy link
Member

6543 commented Jul 13, 2024

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

... so we can just wait till that

@xoxys
Copy link
Member Author

xoxys commented Jul 13, 2024

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.

@6543 6543 added this to the 3.x.x milestone Jul 14, 2024
@qwerty287 qwerty287 modified the milestones: 3.x.x, 3.0.0 Jul 17, 2024
@xoxys
Copy link
Member Author

xoxys commented Aug 3, 2024

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
Copy link
Member Author

xoxys commented Aug 3, 2024

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)

@qwerty287
Copy link
Contributor

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.

@6543
Copy link
Member

6543 commented Sep 5, 2024

ci faild related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking will break existing installations if no manual action happens lib about our woodpecker-go api lib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants