-
Notifications
You must be signed in to change notification settings - Fork 30
feat(test reports): Object storage integration and test-report key #1294
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
base: main
Are you sure you want to change the base?
Conversation
* refactor(pipeline): use server API types for pipeline and migrate compiler types * gci * feat: add sender rule for pipelines --------- Co-authored-by: David May <[email protected]>
* chore(lint): address existing linter issues * remove dupl from exclusion list
* enhance(build): add fork field for OIDC * fix test * integration test update
* enhance(yaml): allow for users to parse pipelines using old library * testing file for internal yaml * chore(compiler): convert unmarshaled buildkite to go-yaml * remove tests used in later PRs * lintfix * fix schema * gci
…1230) Co-authored-by: David May <[email protected]>
Co-authored-by: david may <[email protected]>
* init commit * feat(repo): add pending approval timeout * fix test * remove dead code --------- Co-authored-by: David May <[email protected]>
…r have dupl anchors in map (#1232)
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
golangci
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (invalid statement above assign) (wsl_v5)
server/mock/server/testreport.go
Line 20 in ba647b1
| _ = json.Unmarshal(data, &body) |
🚫 [golangci] reported by reviewdog 🐶
unnecessary whitespace (trailing-whitespace) (wsl_v5)
Line 161 in ba647b1
🚫 [golangci] reported by reviewdog 🐶
unnecessary whitespace (trailing-whitespace) (wsl_v5)
server/router/testattachment.go
Line 20 in ba647b1
🚫 [golangci] reported by reviewdog 🐶
unnecessary whitespace (trailing-whitespace) (wsl_v5)
Line 20 in ba647b1
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (no shared variables above if) (wsl_v5)
| if resp.Code != http.StatusOK { |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (no shared variables above if) (wsl_v5)
| if resp.Code != http.StatusOK { |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (no shared variables above if) (wsl_v5)
| if resp.Code != http.StatusOK { |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (too many statements above defer) (wsl_v5)
server/storage/minio/list_bucket_test.go
Line 36 in ba647b1
| fake := httptest.NewServer(engine) |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (too many statements above defer) (wsl_v5)
server/storage/minio/list_bucket_test.go
Line 71 in ba647b1
| fake := httptest.NewServer(engine) |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (no shared variables above expr) (wsl_v5)
server/storage/minio/list_objects_test.go
Line 66 in ba647b1
| c.Stream(func(w io.Writer) bool { |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (invalid statement above expr) (wsl_v5)
server/storage/minio/list_objects_test.go
Line 71 in ba647b1
| c.XML(http.StatusOK, objects) |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (too many statements above defer) (wsl_v5)
server/storage/minio/list_objects_test.go
Line 76 in ba647b1
| fake := httptest.NewServer(engine) |
🚫 [golangci] reported by reviewdog 🐶
missing whitespace above this line (no shared variables above range) (wsl_v5)
server/storage/minio/list_objects_test.go
Line 234 in ba647b1
| for _, name := range results { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have more of a fundamental piece of feedback and maybe something to open up for discussion (maybe even on the proposal itself). the current proposal didn't really lay out what the integration into a step would look like. it showed one option for implementing this through a custom step/plugin which we didn't pursue here in the end.
as you called out in the description the proposed structure with this PR is:
- name: test-results
image: golang:1.20
test_report:
results: ["test-results/*.xml"]
attachments: [ "cypress/screenshots/**/*.png", "cypress/videos/**/*.mp4"]with test_report and it's associated sub-fields being the main addition here. i am not a fan of the top-level test_report naming for one, partly because it contains multiple words. in addition, it really strongly ties all this to "testing" although the base functionality is more basic - uploading an artifact.
GitLab's implementation, for example, looks something like the following:
job:
script: echo "build xyz project"
artifacts:
paths:
- path/*xyz/*here, artifacts is the top key. a fuller example that includes tests is:
job:
artifacts:
when: always
paths:
- rspec.xml
reports:
junit: rspec.xmlso, paths are the items to upload, and then you can further specify test-specific information through reports.
that matches more with my mental model although i am not saying that our implementation should copy this.
BuildKite has similar functionality also where steps can be defined as follows:
steps:
- label: "🔨 Tests"
command:
- "npm install"
- "tests.sh"
artifact_paths:
- "logs/**/*"
- "coverage/**/*"they chose artifact_paths as their top-level name - also not great, in my opinion but focused on "artifacts". and for their testing support, it looks like you use a custom token and post the test file to a specific BuildKite URL in different ways depending on what language you're using in your project (Go example). seems a little involved, our general approach seems more user friendly.
i just picked two semi-random platforms here, but the point is that i feel like we can improve the setup/naming. to be honest, i'm quite fond of the GitLab approach here. artifacts as a top-level key makes a lot of sense to me, and reports with an object that is <test format>: <path> does give some flexibility there even if we just support one format to start with.
curious if you, or anyone else, have feedback on this part?

Overview
High‑Level Architecture
1. Storage subsystem
Packages & files (high level)
storage/context.go,setup.go,service.go,storage.go(+ tests)flags.go(+ tests) for CLI/env configurationstorage/minio/create_bucket.go,bucket_exists.go,list_bucket.go,get_bucket.goupload.go,list_objects.go,stat_object.go,presigned_get_object.goopts.go,storage_enable.go,minio.gotest_data/create_bucket.json,test_data/test.xmlconstants/driver.go,constants/filetypes.go,constants/table.goDesign
storage/service.go/storage/storage.go), following the sameService + Setup + Contextpattern as other Vela subsystems.storage/minio/.storage/flags.goandstorage/setup.go.storage-enableflag and a storage client into the Gin context.2. Test reports & attachments data model
Database layer
database/reports/testreport/package:create.go,get.go,list.go,list_by_build.go,list_by_repo.go,count.go,count_build.go,count_repo.go,update.go,delete.go,index.go,opts.go,table.go*_test.go).database/reports/testattachment/package:create.go,get.go,get_build.go,list.go,count.go,update.go,delete.go,index.go,opts.go,table.go+ tests.database/types/:test_report.gotest_attachment.godatabase/database.go,database/interface.go, anddatabase/resource.goupdated to:database/testutils/api_resources.goupdated to provide helpers for the new resources.Conceptually
3. API + routing surface
API packages
api/admin/storage.go: admin handlers for bucket creation and presigned URLs for objects.api/storage/doc.go+api/storage/storage.go: public storage handlers (e.g., storage info / health).api/testreport/create.goapi/testreport/update.goapi/testattachment/create.goapi/testattachment/update.goapi/types/test_report.goapi/types/test_attachment.goapi/types/storage.goapi/types/storage_info.goapi/types/pipeline.go/ tests to incorporate the test‑report related shape.Router & middleware
router/storage.go— routes for storage info and/or admin integration.router/testreport.go— routes for test report CRUD & listing.router/testattachment.go— routes for test attachment CRUD & listing.router/admin.go— wires the new admin storage operations.router/build.go— extended to add build‑scoped report/attachment routes.router/middleware/storage.go(+ tests): constructs the storage service, setsstorage-enable, and injects it into Gin.router/middleware/testreport/context.go,testreport/testreport.go: helpers for binding test report resources to requests.router/middleware/testattachment/context.go,testattachment/testattachment.go: same pattern for attachments.Swagger / HTTP endpoints
PUT /api/v1/admin/storage/bucket— create a bucket.GET /api/v1/admin/storage/presign— generate a presigned URL for a given bucket + object.testreportsandtestattachments(see router + API handlers).4. Compiler & pipeline types
compiler/types/pipeline/test_report.go(+ tests) andcompiler/types/yaml/test_report.go:compiler/types/pipeline/container.go,compiler/types/yaml/step.go,compiler/types/yaml/step_test.go:compiler/native/validate.go(+ tests):This sets up the server side so that pipelines can emit test report metadata that is persisted and linked to stored artifacts.
5. Configuration & local dev
cmd/vela-server/main.go,metadata.go,server.go:storage/flags.go,storage/setup.go:storage.Serviceand register it with the router via middleware.docker-compose.ymlandnginx.conf:miniois reachable from the server and UI.DOCS.md:minioto/etc/hosts) and reference storage in the setup flow.Testing
storage/minio/*_test.go).storage/context_test.go,storage/setup_test.go,storage/storage_test.go,storage/flags_test.go).database/reports/testreport/*_test.go,database/reports/testattachment/*_test.go).database/resource_test.go).mock/server/testreport.goadded to support API tests.Example