Refactor: unify API parameters

[zc-devs]

Clear and concise description of the problem

There are API endpoints like:

/api/repos/{repo_id}/logs/{number}/{stepID}

1. Inconsistent naming: repo_id, stepID.
2. Incomprehensible names: number of what?

Suggested solution

Unify parameters names. For example:

/api/repos/{repo_id}/logs/{pipeline_number}/{step_id}
/api/repos/{repoId}/logs/{pipelineNumber}/{stepId}

Alternative

No response

Additional context

No response

Validations

• [X] Checked that the feature isn't part of the next version already [https://woodpecker-ci.org/faq#which-version-of-woodpecker-should-i-use]
• [X] Read the docs.
• [X] Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

[bearrito]

I haven't contributed to this project before. I'll grab this as it looks not-stale and has the good first issue tag.

[qwerty287]

@bearrito What's the state of your work? If you have any questions, just ask :upside_down_face:

[bearrito]

@qwerty287 I had looked at this, and it the scope wasn't obvious. I might be over-complicating it and the scope is smaller than I think

My high-level thinking on this looks like

0. Might have been as simple as changing docs to reflect the naming convention.
1. It seems like docs are generated from code using swagger. There was some tooling around that I didn't look at closely namely the go generate ... calls .
2. Since docs are generated the underlying models need to be change. As an example looking at Repo.
3. Presumably the struct tags for Repo would need to be changed.
4. There around around 240 strings with repo_id in the codebase, it wasn't clear on first look which of these are go to relate to change

Summarizing: There should be some level of abstraction where I can say the change affects things above this poing, but not below, but I couldn't find it.

[zc-devs]

There around around 240 strings

Start from server/router/api.go. It routes to API handlers like server/api/pipeline.go#GetStepLogs. There is the scope.

[qwerty287]

@bearrito It's mainly about unifying the URL params, both in Go code and in swagger docs. I'd recommend using snake_case syntax for the params. You would have to change these files: https://github.com/woodpecker-ci/woodpecker/blob/main/server/router/api.go (containing the params definition in the router) and all under https://github.com/woodpecker-ci/woodpecker/tree/main/server/api (containing both swagger docs and references to the params).

One example from the logs endpoint: https://github.com/woodpecker-ci/woodpecker/blob/7f0084c4769a469a8efa357c4808d23d157baa2a/server/router/api.go#L106 would become:

                    repo.GET("/logs/:number/:step_id", api.GetStepLogs)

This must also be applied here: https://github.com/woodpecker-ci/woodpecker/blob/7f0084c4769a469a8efa357c4808d23d157baa2a/server/api/pipeline.go#L207

    stepID, err := strconv.ParseInt(c.Params.ByName("stepId"), 10, 64)

And for the swagger docs, change https://github.com/woodpecker-ci/woodpecker/blob/7f0084c4769a469a8efa357c4808d23d157baa2a/server/api/pipeline.go#L181 to

//  @Router     /repos/{repo_id}/logs/{number}/{step_id} [get]

and https://github.com/woodpecker-ci/woodpecker/blob/7f0084c4769a469a8efa357c4808d23d157baa2a/server/api/pipeline.go#L188 to

//  @Param      step_id          path    int     true    "the step id"

It seems like docs are generated from code using swagger. There was some tooling around that I didn't look at closely namely the go generate ... calls .

Yes, swagger docs are created from code comments - you can simply use make generate to execute it.

Since docs are generated the underlying models need to be change. As an example looking at Repo. Presumably the struct tags for Repo would need to be changed.

No, changing models isn't required. This is just about URL params. Take a look at the repo model for example: https://github.com/woodpecker-ci/woodpecker/blob/main/server/model/repo.go. There's nothing you need to change as JSON fields are snake_case already.

@zc-devs was a bit faster than me, and this is mainly what I mean in a short form :laughing:

[bearrito]

@qwerty287 That makes sense. That basically what I have at this point. So I guess I'm close.

Related question I was writing unit tests for api.go (It didn't have any and I wanted some guardrails) namely so I could test func apiRoutes(e *gin.RouterGroup). So I have 7-8 of these with mock stores etc

However, attempting to run those gives web.go:24:12: pattern dist/*: no matching files found do you know how I can work around that?

If I comment that out it works, but that's no solution.

[qwerty287]

You can create a dummy file web/dist/index.html without content or use pnpm build inweb/.

[bearrito]

@qwerty287 How would that work on check-in namely with CI?

[qwerty287]

What do you mean? The web/dist dir is not checked in, but the ci also creates the web/dist/index.html file without content.