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

Upgrade jape #742

Merged
merged 16 commits into from
Nov 21, 2023
Merged

Upgrade jape #742

merged 16 commits into from
Nov 21, 2023

Conversation

peterjan
Copy link
Member

@peterjan peterjan commented Nov 15, 2023

jape hasn't been running for some time again and it shows because our clients have a small issue here and there (fixed here. Special thanks to @chris124567 for updating jape so it can handle client definitions in sub-packages and extending the GH action to support passing in directories

@peterjan peterjan self-assigned this Nov 15, 2023
@peterjan peterjan marked this pull request as ready for review November 15, 2023 18:12
@chris124567
Copy link
Member

chris124567 commented Nov 15, 2023

That's odd. Before we merged the jape changes I tested it on a copy of renterd at https://github.com/test5000009/renterd/actions/runs/6747834229 and it found the unused routes in bus. Will look into this.

Edit:

/opt/hostedtoolcache/go/1.21.3/x64/bin/go get golang.org/x/tools/go/analysis/multichecker go.sia.tech/jape@HEAD
...
go: added go.sia.tech/jape v0.10.0

Looks like jape v0.10.0 is being used which predates my changes (despite the fact that v0.11.0 was published and should work). The analyzer adds @Head if a version isn't requested so it should be getting the latest version.

@chris124567
Copy link
Member

I got this configuration:

      - name: Check Endpoints
        uses: SiaFoundation/action-golang-analysis@HEAD
        with:
          analyzers: |
            go.sia.tech/jape.Analyzer@master
          directories: |
            autopilot
            bus bus/client
            worker worker/client

working on my test repository, using @master instead of @HEAD. I'm not sure why Go is choosing an old version when @HEAD is specified. I don't want to make the analyzer add @master instead of @HEAD because some packages are using main instead of master nowadays whereas specifying @HEAD should always work. Do Go module proxies have some kind of cache?

@peterjan
Copy link
Member Author

Thanks @chris124567 - took me one hour this morning to come to that same conclusion you left on my PR 🤣

@peterjan
Copy link
Member Author

peterjan commented Nov 16, 2023

Hm @chris124567 it seems that the jape analyzer has an NDF now where it claims the client is missing methods for every single route. Any clue why that might be? If I run watch -n 2 japecheck ./bus ./bus/client locally it happens in like 10-20% of all runs.

  Error: /home/runner/work/renterd/renterd/bus/client/chain.go:19:9: Client references route not defined by server: POST /txpool/broadcast
  Error: /home/runner/work/renterd/renterd/bus/client/chain.go:44:8: Client references route not defined by server: GET 

https://github.com/SiaFoundation/renterd/actions/runs/6891677726/job/18747258490?pr=742

@chris124567
Copy link
Member

chris124567 commented Nov 16, 2023

I got:

/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:22:34: route contains (1 path + 0 form) = 1 parameters, but 2 arguments are supplied
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:44:34: route contains (1 path + 0 form) = 1 parameters, but 2 arguments are supplied
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:69:34: route contains (1 path + 0 form) = 1 parameters, but 2 arguments are supplied
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/bus.go:2038:13: Encode called on []go.sia.tech/renterd/api.ContractSetMetric, but was previously called on []go.sia.tech/renterd/api.ContractMetric
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/bus.go:2052:13: Encode called on []go.sia.tech/renterd/api.ContractSetChurnMetric, but was previously called on []go.sia.tech/renterd/api.ContractMetric
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:22:104: Client has wrong response type for GET /metric/:key (got *[]go.sia.tech/renterd/api.ContractSetMetric, should be *[]go.sia.tech/renterd/api.ContractMetric)
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:22:9: Client has too few path parameters for GET /metric/:key
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:44:9: Client references GET /metric/%s?%s multiple times
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:69:9: Client references GET /metric/%s?%s multiple times

on latest renterd. But if I run it multiple times I sometimes get your output. Looks like this is a concurrency related issue (when multiple directories are passed they are are all analyzed in parallel). If I run jape built locally with -race I get a bunch of race condition warnings - I think we should just revert SiaFoundation/jape@148b9cf which based on my testing is where this bug was introduced. I'll make a PR.

@chris124567
Copy link
Member

chris124567 commented Nov 16, 2023

OK I made SiaFoundation/jape#16. On that branch I get the same output 100% of the time and no races. Sorry for the bug - guess I just got lucky in testing because it didn't show up then.

+ go build -race -o check ./japecheck
+ cd ../renterd
+ ../jape/check ./bus ./bus/client
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:22:34: route contains (1 path + 0 form) = 1 parameters, but 2 arguments are supplied
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:44:34: route contains (1 path + 0 form) = 1 parameters, but 2 arguments are supplied
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:69:34: route contains (1 path + 0 form) = 1 parameters, but 2 arguments are supplied
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/bus.go:2038:13: Encode called on []go.sia.tech/renterd/api.ContractSetMetric, but was previously called on []go.sia.tech/renterd/api.ContractMetric
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/bus.go:2052:13: Encode called on []go.sia.tech/renterd/api.ContractSetChurnMetric, but was previously called on []go.sia.tech/renterd/api.ContractMetric
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:22:104: Client has wrong response type for GET /metric/:key (got *[]go.sia.tech/renterd/api.ContractSetMetric, should be *[]go.sia.tech/renterd/api.ContractMetric)
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:22:9: Client has too few path parameters for GET /metric/:key
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:44:9: Client references GET /metric/%s?%s multiple times
/home/christopher/prog/go/src/go.sia.tech/renterd/bus/client/metrics.go:69:9: Client references GET /metric/%s?%s multiple times

@chris124567
Copy link
Member

... there we go.

@ChrisSchinnerl ChrisSchinnerl merged commit ff89809 into master Nov 21, 2023
6 checks passed
@ChrisSchinnerl ChrisSchinnerl deleted the pj/jape branch November 21, 2023 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants