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

feat: Refactors API client into hand rolled sdk in api/ directory #111

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

jpmcb
Copy link
Member

@jpmcb jpmcb commented Aug 29, 2024

Description

Refactors API client code into a hand rolled SDK under api/. This is partially for simplicity but also so we can move fast without having to roll out a new generated Go SDK.

This also refactors several of the broken uses of the v2 API. Now, the old pizza insights commands work.

Related Tickets & Documents

Closes: #87

Steps to QA

  1. just build
  2. just lint
  3. just test
  4. Able to run all the following commands:
    • ./build/pizza generate codeowners .
    • ./build/pizza insights user-contributions
    • ./build/pizza insights repositories
    • ./build/pizza insights contributors

Tier (staff will fill in)

  • Tier 1
  • Tier 2
  • Tier 3
  • Tier 4

@jpmcb jpmcb force-pushed the api-client-refactor branch from 2685c96 to bcddef0 Compare August 29, 2024 01:33
@jpmcb jpmcb marked this pull request as draft August 29, 2024 01:35
@jpmcb jpmcb changed the title feat: API client code refactor feat: Refactors API client into hand rolled sdk in api/ directory Aug 29, 2024
@jpmcb jpmcb changed the title feat: Refactors API client into hand rolled sdk in api/ directory feat: Refactors API client into hand rolled sdk in api/ directory Aug 29, 2024
@jpmcb jpmcb force-pushed the api-client-refactor branch from bcddef0 to 337fd4a Compare September 3, 2024 15:33
@jpmcb jpmcb force-pushed the api-client-refactor branch from 337fd4a to ff7eba0 Compare September 3, 2024 17:43
@jpmcb jpmcb marked this pull request as ready for review September 3, 2024 17:45
@jpmcb jpmcb requested a review from a team September 3, 2024 17:46
Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

I get the following error when running just lint. I also made sure I have all the deps, by running go mod download. It seems to just be the linting.

WARN [runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/cpuguy83/go-md2man/v2/md2man" 
INFO [runner] processing took 1.877µs with stages: max_same_issues: 334ns, invalid_issue: 250ns, source_code: 167ns, filename_unadjuster: 167ns, cgo: 167ns, sort_results: 125ns, identifier_marker: 83ns, path_prefixer: 42ns, skip_dirs: 42ns, path_shortener: 42ns, skip_files: 42ns, fixer: 42ns, nolint: 42ns, path_prettifier: 42ns, exclude: 42ns, max_from_linter: 42ns, severity-rules: 42ns, max_per_file_from_linter: 41ns, autogenerated_exclude: 41ns, exclude-rules: 41ns, diff: 41ns, uniq_by_line: 0s 
INFO [runner] linters took 1.844725876s with stages: goanalysis_metalinter: 1.844699918s 
ERRO Running error: can't run linter goanalysis_metalinter
buildir: failed to load package : could not load export data: no export data for "github.com/cpuguy83/go-md2man/v2/md2man" 
INFO Memory: 66 samples, avg is 137.4MB, max is 518.7MB 
INFO Execution took 6.436220378s                  
error: Recipe `lint` failed on line 159 with exit code 3

@nickytonline
Copy link
Member

nickytonline commented Sep 3, 2024

I get the following error when running just lint. I also made sure I have all the deps, by running go mod download. It seems to just be the linting.

WARN [runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/cpuguy83/go-md2man/v2/md2man" 
INFO [runner] processing took 1.877µs with stages: max_same_issues: 334ns, invalid_issue: 250ns, source_code: 167ns, filename_unadjuster: 167ns, cgo: 167ns, sort_results: 125ns, identifier_marker: 83ns, path_prefixer: 42ns, skip_dirs: 42ns, path_shortener: 42ns, skip_files: 42ns, fixer: 42ns, nolint: 42ns, path_prettifier: 42ns, exclude: 42ns, max_from_linter: 42ns, severity-rules: 42ns, max_per_file_from_linter: 41ns, autogenerated_exclude: 41ns, exclude-rules: 41ns, diff: 41ns, uniq_by_line: 0s 
INFO [runner] linters took 1.844725876s with stages: goanalysis_metalinter: 1.844699918s 
ERRO Running error: can't run linter goanalysis_metalinter
buildir: failed to load package : could not load export data: no export data for "github.com/cpuguy83/go-md2man/v2/md2man" 
INFO Memory: 66 samples, avg is 137.4MB, max is 518.7MB 
INFO Execution took 6.436220378s                  
error: Recipe `lint` failed on line 159 with exit code 3

It looks like running go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest fixed the issue. I ran go mod tidy after that, but no changes. It looks lie golangci-lint isn't in go.mod. Are linter tools in go land expected to be only on your local and not in the project?

Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

Tested all the commands and they ran successfully. Just some questions.

CleanShot 2024-09-03 at 17 25 52

Not related to this PR, but the CODEOWNERS generation strips usernames from the CODEOWNERS much like I exhibited when running the pizza-action in the app repo, and beta for the pizza-cli locally behaves that way as well.

cmd/insights/repositories.go Show resolved Hide resolved
@jpmcb
Copy link
Member Author

jpmcb commented Sep 3, 2024

Are linter tools in go land expected to be only on your local and not in the project?

No. There's one unified toolchain in the go command line. golangci-lint is an extra piece of sugar and has no integration into the Go toolchain. Think of it as an extra, optional opinion: as long as the go build or go run commands work, technically, it's completely valid Go.

@jpmcb
Copy link
Member Author

jpmcb commented Sep 3, 2024

It looks like running go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest

That's very, very odd: the just lint command runs a container from their registry tagged v1.59:

pizza-cli/justfile

Lines 152 to 159 in ad9e3a0

lint:
docker run \
-t \
--rm \
-v "$(pwd)/:/app" \
-w /app \
golangci/golangci-lint:v1.59 \
golangci-lint run -v

Also note the successful lint run on this PR - maybe some useful logs there: https://github.com/open-sauced/pizza-cli/actions/runs/10687979965/job/29626781164?pr=111

The local code directory is mounted as /app and the linting is run in a totally isolated environment. Do you consistently get that error? Even off beta or main?

You might also try to upgrade the linter locally. Looks like they cut a 1.60:

diff --git a/justfile b/justfile
index a0e24e0..f47a1ac 100644
--- a/justfile
+++ b/justfile
@@ -155,7 +155,7 @@ lint:
     --rm \
     -v "$(pwd)/:/app" \
     -w /app \
-    golangci/golangci-lint:v1.59 \
+    golangci/golangci-lint:v1.60 \
     golangci-lint run -v

 # Formats code via builtin go fmt

@nickytonline
Copy link
Member

It looks like running go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest

That's very, very odd: the just lint command runs a container from their registry tagged v1.59:

pizza-cli/justfile

Lines 152 to 159 in ad9e3a0

lint:
docker run \
-t \
--rm \
-v "$(pwd)/:/app" \
-w /app \
golangci/golangci-lint:v1.59 \
golangci-lint run -v

Also note the successful lint run on this PR - maybe some useful logs there: open-sauced/pizza-cli/actions/runs/10687979965/job/29626781164?pr=111

The local code directory is mounted as /app and the linting is run in a totally isolated environment. Do you consistently get that error? Even off beta or main?

You might also try to upgrade the linter locally. Looks like they cut a 1.60:

diff --git a/justfile b/justfile
index a0e24e0..f47a1ac 100644
--- a/justfile
+++ b/justfile
@@ -155,7 +155,7 @@ lint:
     --rm \
     -v "$(pwd)/:/app" \
     -w /app \
-    golangci/golangci-lint:v1.59 \
+    golangci/golangci-lint:v1.60 \
     golangci-lint run -v

 # Formats code via builtin go fmt

My bad. Docker for Desktop wasn't completely running as I don't have it on by default. All good now.

pizza-cli on  api-client-refactor [$] via 🐳 desktop-linux via 🐹 v1.23.0 
❯ just lint
docker run -t --rm -v "$(pwd)/:/app" -w /app golangci/golangci-lint:v1.59 golangci-lint run -v
INFO golangci-lint has version 1.59.1 built with go1.22.3 from 1a55854a on 2024-06-09T18:08:33Z 
INFO [config_reader] Config search paths: [./ /app / /root] 
INFO [config_reader] Used config file .golangci.yaml 
INFO [lintersdb] Active 11 linters: [errcheck gci goimports gosimple govet ineffassign misspell revive staticcheck unconvert unused] 
INFO [loader] Go packages loading at mode 575 (deps|exports_file|types_sizes|name|compiled_files|files|imports) took 5.275550502s 
INFO [runner/filename_unadjuster] Pre-built 0 adjustments in 3.300542ms 
INFO [linters_context/goanalysis] analyzers took 10.827503202s with top 10 stages: buildir: 6.74468295s, the_only_name: 1.403188459s, inspect: 420.207254ms, fact_deprecated: 349.529408ms, printf: 277.186624ms, ctrlflow: 274.731882ms, nilness: 153.976679ms, fact_purity: 132.305999ms, gci: 113.724707ms, SA5012: 112.346914ms 
INFO [runner] Issues before processing: 57, after processing: 0 
INFO [runner] Processors filtering stat (out/in): skip_dirs: 57/57, exclude: 57/57, cgo: 57/57, filename_unadjuster: 57/57, invalid_issue: 57/57, identifier_marker: 57/57, skip_files: 57/57, autogenerated_exclude: 57/57, exclude-rules: 4/57, nolint: 0/4, path_prettifier: 57/57 
INFO [runner] processing took 4.082544ms with stages: autogenerated_exclude: 2.373792ms, exclude-rules: 740.666µs, identifier_marker: 555.458µs, nolint: 196.875µs, path_prettifier: 143.209µs, skip_dirs: 62.958µs, cgo: 2.792µs, invalid_issue: 2.125µs, filename_unadjuster: 1.958µs, max_same_issues: 667ns, fixer: 334ns, sort_results: 291ns, exclude: 209ns, skip_files: 208ns, uniq_by_line: 208ns, diff: 167ns, max_from_linter: 167ns, severity-rules: 126ns, max_per_file_from_linter: 125ns, path_shortener: 84ns, source_code: 83ns, path_prefixer: 42ns 
INFO [runner] linters took 1.825440167s with stages: goanalysis_metalinter: 1.821289542s 
INFO File cache stats: 45 entries of total size 98.8KiB 
INFO Memory: 73 samples, avg is 143.8MB, max is 592.3MB 
INFO Execution took 7.105187379s          

Copy link
Member

@nickytonline nickytonline left a comment

Choose a reason for hiding this comment

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

The CODEOWNERS issue isn't related to this so going to go ahead and ✅

@jpmcb jpmcb merged commit e16e889 into beta Sep 4, 2024
21 checks passed
@jpmcb jpmcb deleted the api-client-refactor branch September 4, 2024 16:04
open-sauced bot pushed a commit that referenced this pull request Sep 4, 2024
## [1.3.0-beta.1](v1.2.1...v1.3.0-beta.1) (2024-09-04)

### 🍕 Features

* Refactors API client into hand rolled sdk in api/ directory ([#111](#111)) ([e16e889](e16e889))
open-sauced bot pushed a commit that referenced this pull request Sep 6, 2024
## [1.3.0](v1.2.1...v1.3.0) (2024-09-06)

### 🍕 Features

* Create a contributor list after generating codeowners ([#141](#141)) ([72c5d58](72c5d58))
* now the documentation for the pizza-cli can be generated via pizza docs ([#143](#143)) ([3f5d27e](3f5d27e))
* Refactors API client into hand rolled sdk in api/ directory ([#111](#111)) ([e16e889](e16e889))
* support fallback attributions when generating codeowners file ([#145](#145)) ([35af4da](35af4da))
* update `CODEOWNERS` copy with command ([#130](#130)) ([a477959](a477959))

### 🐛 Bug Fixes

* Corrects invalid gosec lint error ([#151](#151)) ([f76527f](f76527f))
* Exhume Posthog functionality ([#147](#147)) ([de091ca](de091ca))
* now fallback .sauced.yaml contents get read ([#135](#135)) ([fd658e5](fd658e5))
* NPM cache now looks at package-lock file ([#136](#136)) ([cd4b8da](cd4b8da))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: replace generated Go SDK library with hand rolled SDK
3 participants