feat(cli): migrate e2e tests and fix lint errors#3236
feat(cli): migrate e2e tests and fix lint errors#3236alkalescent merged 4 commits intoDSPX-2655-migrate-otdfctlfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on modernizing the CI/CD pipeline for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The linter cried out in the night, / To set all the code paths aright. / With local actions now in place, / The build runs at a faster pace, / And all the logs are shining bright. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request performs a comprehensive refactoring of the otdfctl CLI codebase. Key changes include replacing fmt.Errorf with errors.New for static error messages, updating logging to use structured slog calls, and standardizing error messages and string concatenations. Additionally, the pull request updates the .golangci.yaml configuration to exclude specific paths during migration and includes a fix for a typo in an error message within otdfctl/pkg/cli/pipe.go.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Dependency ReviewThe following issues were found:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
07bc639 to
5e4def8
Compare
…rrors Rewrite the otdfctl e2e test CI to use local composite action references instead of cross-repo checkouts from opentdf/otdfctl, since otdfctl now lives in the platform monorepo. - Update checks.yaml otdfctl-test job to checkout platform repo and reference ./test/start-up-with-containers and ./otdfctl/e2e locally - Update otdfctl/e2e/action.yaml: remove external checkout step, remove otdfctl-ref input, use otdfctl/v0.26.2 subtree tag for legacy binary - Update nightly-checks.yaml to build otdfctl from platform/otdfctl/ instead of checking out opentdf/otdfctl separately - Restore tui/ directory lint exclusion (matching original otdfctl config) - Fix ~60 lint errors: gofumpt formatting, unused nolint directives, perfsprint (fmt.Errorf->errors.New, fmt.Sprintf->string concat), sloglint (lowercase messages, attr-only, snake_case keys, args on separate lines), revive (indent-error-flow, blank import comment) - Add golangci.yaml exclusions for deferred refactoring-level fixes (contextcheck, revive unused-parameter/unexported-return/var-naming) Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
3aa54bb to
02d6a18
Compare
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
| run: | | ||
| sudo apt update | ||
| sudo apt install -y parallel | ||
| - uses: opentdf/platform/test/start-up-with-containers@main |
There was a problem hiding this comment.
Since we now explicitly check out the repo for the local e2e otdfctl step, we can use the local version of the action too. Otherwise, we'd have two checkouts (including the implicit one when you do use: *@main) and we wouldn't be able to test start-up-with-containers action changes (not relevant here). Down to revert this specific change in my next PR to the stacked branch if someone has a strong opinion.
| - uses: opentdf/otdfctl/e2e@main | ||
| with: | ||
| otdfctl-ref: "main" | ||
| - uses: ./otdfctl/e2e |
There was a problem hiding this comment.
Will this change to opentdf/platform/otdfctl@main after mergning?
There was a problem hiding this comment.
I'm thinking not — to ensure breaking changes are caught before merging to main.
- Rewrites the `otdfctl-test` CI job in `checks.yaml` to use local composite action references (`./test/start-up-with-containers`, `./otdfctl/e2e`) instead of cross-repo checkouts from `opentdf/otdfctl` - Updates `otdfctl/e2e/action.yaml`: removes external checkout step, removes `otdfctl-ref` input, uses `otdfctl/v0.26.2` subtree tag for legacy binary build - Updates `nightly-checks.yaml` to build otdfctl from `platform/otdfctl/` instead of checking out `opentdf/otdfctl` separately - Restores `tui/` directory lint exclusion (matching original otdfctl config) and fixes ~60 lint errors (gofumpt, unused nolint directives, perfsprint, sloglint, revive) - Adds `.golangci.yaml` exclusion rules for deferred refactoring-level fixes (contextcheck, revive unused-parameter/unexported-return/var-naming) Resolves [DSPX-2659](https://virtru.atlassian.net/browse/DSPX-2659) - [ ] `otdfctl-test` CI job passes (all 19 BATS e2e test files) - [ ] Legacy v0.26.2 binary build succeeds via `git worktree add ../otdfctl_v0.26.2 otdfctl/v0.26.2` - [ ] Profile keyring tests pass with legacy binary - [ ] `golangci-lint run ./...` from `otdfctl/` passes with 0 issues - [ ] `go test ./...` from `otdfctl/` passes - [ ] Nightly-checks workflow syntax is valid [DSPX-2659]: https://virtru.atlassian.net/browse/DSPX-2659?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
### Proposed Changes * Add otdfctl component to platform release-please configuration for independent versioned releases * Tags follow the monorepo per-component pattern: `otdfctl/v0.30.0` * Register `otdfctl/pkg/config/config.go` as extra-file so release-please bumps the `Version` constant (already has `// x-release-please-version` marker) * Create release workflow that triggers on `otdfctl/v*` tags, builds 8 cross-platform binaries (darwin amd64/arm64, linux amd64/arm/arm64, windows amd64/arm/arm64), and uploads artifacts to the GitHub release #### Files added/modified | File | Change | |------|--------| | `release-please-config.main.json` | Add `otdfctl` package entry with `extra-files` | | `release-please-manifest.json` | Add `"otdfctl": "0.30.0"` version tracking | | `release-please-config.otdfctl.json` | **New** — component config for `release/otdfctl/vX.Y` branches | | `release-otdfctl.yaml` | **New** — build and upload workflow on release publish | #### PR Stack (DSPX-2654) 1. #3205 — Subtree merge + module path rewrite (DSPX-2655, DSPX-2656) 2. #3208 — Makefile and build scripts (DSPX-2657) 3. #3221 — CI workflows (DSPX-2658) 4. #3236 — e2e tests and lint fixes (DSPX-2659) 5. **This PR** — Release pipeline (DSPX-2660) ### Checklist - [ ] I have added or updated unit tests - [x] I have added or updated integration tests (if appropriate) - [x] I have added or updated documentation ### Testing Instructions - Verify JSON configs are valid: `cat .github/release-please/release-please-config.main.json | jq .packages.otdfctl` - Verify manifest version: `cat .github/release-please/release-please-manifest.json | jq .otdfctl` - Verify `reusable_release-please.yaml` config lookup: branch `release/otdfctl/v0.30` → sanitized name `otdfctl` → resolves to `release-please-config.otdfctl.json` - Full release flow testable after merge by creating a manual release with tag `otdfctl/v0.30.0` --------- Signed-off-by: Krish Suchak <suchak.krish@gmail.com>
Summary
otdfctl-testCI job inchecks.yamlto use local composite action references (./test/start-up-with-containers,./otdfctl/e2e) instead of cross-repo checkouts fromopentdf/otdfctlotdfctl/e2e/action.yaml: removes external checkout step, removesotdfctl-refinput, usesotdfctl/v0.26.2subtree tag for legacy binary buildnightly-checks.yamlto build otdfctl fromplatform/otdfctl/instead of checking outopentdf/otdfctlseparatelytui/directory lint exclusion (matching original otdfctl config) and fixes ~60 lint errors (gofumpt, unused nolint directives, perfsprint, sloglint, revive).golangci.yamlexclusion rules for deferred refactoring-level fixes (contextcheck, revive unused-parameter/unexported-return/var-naming)Resolves DSPX-2659
Test plan
otdfctl-testCI job passes (all 19 BATS e2e test files)git worktree add ../otdfctl_v0.26.2 otdfctl/v0.26.2golangci-lint run ./...fromotdfctl/passes with 0 issuesgo test ./...fromotdfctl/passes