Conversation
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds V3 API end-to-end Cypress test coverage for Users, Health, Documentation, Organization, and Version endpoints; implements defensive nil-checks in backend handlers; extends AWS claims management to us-east-2 region; updates test execution utilities to support conditional token loading and multi-file test specification. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas requiring extra attention:
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR extends multi-region support for AWS SSM parameters across development and production environments, and adds comprehensive V3 API test coverage. The changes ensure authentication claims are managed consistently in both us-east-1 and us-east-2 regions, while also improving error handling for user retrieval operations.
- Added us-east-2 region support for AWS SSM parameter operations across dev and prod environments
- Introduced V3 API test files for version, users, organization, health, and documentation endpoints
- Enhanced test runner script to support running all tests in a specific API version
- Improved null safety in user retrieval handler
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/set_dev_claims.sh | Adds us-east-2 region SSM parameter creation commands for dev environment |
| utils/restore_dev_claims.sh | Adds us-east-2 region SSM parameter restoration and deletion commands for dev environment |
| utils/get_prod_claims.sh | Adds us-east-2 region SSM parameter retrieval commands for prod environment |
| utils/get_dev_claims.sh | Adds us-east-2 region SSM parameter retrieval commands for dev environment |
| tests/functional/utils/run-single-test.sh | Updates help text and implements logic to run all tests for a specific API version |
| tests/functional/cypress/e2e/v3/version.cy.ts | New test file for V3 version API endpoint |
| tests/functional/cypress/e2e/v3/users.cy.ts | New test file for V3 user API endpoints |
| tests/functional/cypress/e2e/v3/organization.cy.ts | New test file for V3 organization API endpoints |
| tests/functional/cypress/e2e/v3/health.cy.ts | New test file for V3 health API endpoint |
| tests/functional/cypress/e2e/v3/docs.cy.ts | New test file for V3 documentation API endpoints |
| cla-backend-go/users/handlers.go | Adds null check for user model and improves error message when user not found |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
tests/functional/utils/run-single-test.sh (1)
39-48: Dynamic spec list generation works correctly.The loop properly builds a comma-separated list of all test specs for the selected version. The trailing space check ensures proper comma placement.
Consider this slightly simpler approach:
- CMD="xvfb-run -a npx cypress run --spec " - for file in cypress/e2e/v${V}/*.cy.ts - do - if [ "${CMD: -1}" = " " ] - then - CMD="${CMD}${file}" - continue - fi - CMD="${CMD},${file}" - done + specs="" + for file in cypress/e2e/v${V}/*.cy.ts; do + specs="${specs:+$specs,}${file}" + done + CMD="xvfb-run -a npx cypress run --spec ${specs}"tests/functional/cypress/e2e/v3/organization.cy.ts (1)
24-71: Consider consolidating duplicate test logic.These three tests share nearly identical structure and validation logic. Consider refactoring into a parameterized test to reduce duplication.
Example approach:
it('Search Organization with various parameters - Record should return 200 Response', function () { const cases = [ { title: 'by company name', params: { companyName: 'Linux Foundation' } }, { title: 'by website name', params: { websiteName: 'linuxfoundation.org' } }, { title: 'by both', params: { companyName: 'Linux Foundation', websiteName: 'linuxfoundation.org' } }, ]; cy.wrap(cases).each((c: any) => { const queryString = new URLSearchParams(c.params).toString(); cy.task('log', `Testing organization search ${c.title}`); return cy.request({ method: 'GET', url: `${claEndpoint}organization/search?${queryString}`, timeout: timeout, failOnStatusCode: allowFail, }).then((response) => { validate_200_Status(response); expect(response.body).to.be.an('object'); if (response.body.list) { expect(response.body.list).to.be.an('array'); } }); }); });tests/functional/cypress/e2e/v3/users.cy.ts (1)
79-107: Consider documenting the test user ID and expected behavior.The hardcoded user ID
d9428888-122b-4b20-8c4a-0c9a1a6f9b8eis used without explanation. Document whether this user is expected to exist (200), not exist (404), or either is acceptable. This will make the test intent clearer and help with maintenance.Additionally, accepting 401 for an authenticated request suggests auth infrastructure issues that should be tracked separately rather than embedded in functional tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
cla-backend-go/users/handlers.go(1 hunks)tests/functional/cypress/e2e/v3/docs.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/health.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/organization.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/users.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/version.cy.ts(1 hunks)tests/functional/utils/run-single-test.sh(2 hunks)utils/get_dev_claims.sh(1 hunks)utils/get_prod_claims.sh(1 hunks)utils/restore_dev_claims.sh(1 hunks)utils/set_dev_claims.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
tests/functional/cypress/e2e/v3/health.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/docs.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/organization.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/users.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeaders(268-280)validate_200_Status(47-52)
tests/functional/cypress/e2e/v3/version.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)validate_200_Status(47-52)validate_expected_status(155-198)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL analysis (go)
- GitHub Check: cypress-functional
- GitHub Check: build-test-lint
🔇 Additional comments (14)
cla-backend-go/users/handlers.go (1)
260-263: Good defensive nil check.This prevents a potential nil pointer dereference when
service.GetUser()returnsnil, nil. The handler now properly treats a missing user as an error condition before attempting conversion.utils/restore_dev_claims.sh (1)
7-10: Multi-region support looks correct.The us-east-2 commands properly mirror the us-east-1 setup for restoring dev claims. Parameter names and values are consistent.
utils/get_prod_claims.sh (1)
6-8: Multi-region support implemented correctly.The us-east-2 commands for username and email claims are properly formatted.
utils/get_dev_claims.sh (1)
6-9: Multi-region support looks correct.The us-east-2 commands properly mirror the us-east-1 setup for fetching dev claims. All parameter names are consistent.
utils/set_dev_claims.sh (1)
6-8: Multi-region setup is consistent.The us-east-2 commands correctly replicate the us-east-1 parameter values for dev claim configuration.
tests/functional/utils/run-single-test.sh (1)
24-25: Help text updated for V3 and V4 API groups.The examples now clearly document the available test groups for both V3 and V4 APIs, which aligns with the new V3 test suite additions.
tests/functional/cypress/e2e/v3/version.cy.ts (1)
1-87: Well-structured V3 Version API tests.The test suite provides good coverage of both success and failure scenarios:
- Validates successful version endpoint response structure
- Tests method-not-allowed errors appropriately
- Follows established patterns from other V3 test suites
- Proper use of shared validation helpers
tests/functional/cypress/e2e/v3/docs.cy.ts (1)
1-108: Comprehensive V3 Documentation API tests.The test suite thoroughly validates documentation endpoints:
- Covers both
/api-docsand/swagger.jsonendpoints- Validates swagger.json structure (swagger, info, paths)
- Tests method-not-allowed scenarios appropriately
- Follows consistent patterns with other V3 test suites
tests/functional/cypress/e2e/v3/health.cy.ts (2)
25-37: LGTM!Good test coverage for the health endpoint with appropriate assertions for status and response structure.
39-84: LGTM!The parameterized approach to testing expected failures is well-structured and makes it easy to add additional negative test cases in the future.
tests/functional/cypress/e2e/v3/organization.cy.ts (1)
73-124: LGTM!Good coverage of error scenarios with appropriate status code and message validation.
tests/functional/cypress/e2e/v3/users.cy.ts (3)
24-52: Consider more specific test assertions.Accepting both 200 and 401 as valid responses reduces the test's ability to detect auth regressions. While the comment acknowledges "auth issues," consider whether this test should:
- Be marked as
skip()until infrastructure is stable- Have separate tests for authenticated (expects 200) and unauthenticated (expects 401) scenarios
- Be moved to a "known issues" test suite
This pattern undermines confidence in the test suite's ability to catch real problems.
109-136: LGTM!Good security testing to verify that endpoints properly require authentication and return 401 for unauthenticated requests.
54-77: Remove the review comment—the test is correctly designed for known infrastructure issues.The test's approach is intentional and established throughout your codebase. It explicitly documents known infrastructure issues (line 62) and uses environment-aware validation that many other tests also employ (
if (local)pattern found in projects.cy.ts, metrics.cy.ts, signatures.cy.ts, etc.).Key points:
- The hardcoded UUID is a standard test fixture used across 10+ test files (not special)
- Status code acceptance differs by environment intentionally: local allows 400/500/502 due to documented connection issues, remote expects fewer errors
- The test still validates successful responses (line 73: checks response.body is an object when status=200)
- This pragmatic pattern handles known infrastructure problems while maintaining coverage for valid responses
Likely an incorrect or invalid review comment.
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (5)
12-12: Clarify theallowFaillogic.The expression
!(Cypress.env('ALLOW_FAIL') === 1)means whenALLOW_FAILis1,allowFailwill befalse. This is counterintuitive. Consider renaming or inverting the logic for clarity.Apply this diff to improve clarity:
- let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); + let failOnStatusCode: boolean = Cypress.env('ALLOW_FAIL') !== 1;
14-14: Remove unused variable.The
localvariable is declared but never used in this file.Apply this diff:
- const local = Cypress.env('LOCAL');
30-55: Improve test determinism.The test accepts multiple status codes (200, 401, 403, 404) without clear pass/fail criteria. This makes the test non-deterministic and reduces its value. A test should have predictable expectations based on the test data setup.
Consider:
- Using test data that's guaranteed to exist in the test environment, or
- Setting up the necessary test data in a
before()hook, or- Clearly defining which status code is expected and treating others as failures
For example:
- }).then((response) => { - if (response.status === 200) { - validate_200_Status(response); - expect(response.body).to.be.an('object'); - if (response.body.requests) { - expect(response.body.requests).to.be.an('array'); - } - } else if (response.status === 404) { - // Company or project not found is acceptable for this test - expect(response.status).to.eq(404); - } else { - // Allow other statuses during development - expect([200, 401, 403, 404]).to.include(response.status); - } + }).then((response) => { + // For WIP: document expected behavior once test data is set up + validate_200_Status(response); + expect(response.body).to.be.an('object'); + if (response.body.requests) { + expect(response.body.requests).to.be.an('array'); + } });
57-79: Improve test determinism.Same issue as the previous test - accepting multiple status codes (200, 401, 403, 404) makes the test non-deterministic. Once you have proper test data setup, this test should have a single, clear expected outcome.
117-127: Remove unused test case properties.The test case type defines
expectedCode,expectedMessage, andexpectedMessageContainsproperties, but none of these are used in the actual test assertions (lines 154-179). Either implement validation for these properties or remove them from the type definition.If you want to validate error messages, apply this diff:
.then((response) => { cy.task('log', `Testing: ${c.title} - Got status: ${response.status}`); if (Array.isArray(c.expectedStatus)) { expect(c.expectedStatus).to.include(response.status); } else if (c.expectedStatus) { expect(response.status).to.eq(c.expectedStatus); } + if (c.expectedMessage && response.body) { + if (c.expectedMessageContains) { + expect(response.body.message).to.include(c.expectedMessage); + } else { + expect(response.body.message).to.eq(c.expectedMessage); + } + } });Otherwise, remove the unused properties from the type definition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts(1 hunks)utils/get_prod_claims.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/get_prod_claims.sh
🧰 Additional context used
🧬 Code graph analysis (1)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
tests/functional/cypress/support/commands.js (6)
getAPIBaseURL(236-253)local(237-237)bearerToken(288-288)getTokenKey(289-322)getXACLHeaders(268-280)validate_200_Status(47-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cypress-functional
- GitHub Check: build-test-lint
🔇 Additional comments (1)
tests/functional/cypress/e2e/v3/cla-manager.cy.ts (1)
81-109: LGTM - Authentication tests are well-structured.The authentication required tests have clear expectations and properly validate that requests without bearer tokens return 401 with appropriate error messages.
Signed-off-by: Lukasz Gryglicki <lgryglicki@cncf.io> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tests/functional/cypress/e2e/v3/docs.cy.ts (1)
7-7: Remove unused variable.The
localvariable is declared but never used in this test file.Apply this diff:
const claEndpoint = getAPIBaseURL('v3'); let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); const timeout = 180000; - const local = Cypress.env('LOCAL');tests/functional/cypress/e2e/v3/organization.cy.ts (2)
1-6: Remove unused import.The
validate_401_Statusimport is never used in this file.Apply this diff:
import { validate_200_Status, - validate_401_Status, getAPIBaseURL, validate_expected_status, } from '../../support/commands';
12-12: Remove unused variable.The
localvariable is declared but never used in this test file.Apply this diff:
const claEndpoint = getAPIBaseURL('v3'); let allowFail: boolean = !(Cypress.env('ALLOW_FAIL') === 1); const timeout = 180000; - const local = Cypress.env('LOCAL');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
cla-backend-go/company/handlers.go(1 hunks)tests/functional/cypress/e2e/v3/docs.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/health.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/organization.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/users.cy.ts(1 hunks)tests/functional/cypress/e2e/v3/version.cy.ts(1 hunks)tests/functional/cypress/support/commands.js(1 hunks)tests/functional/utils/run-single-test-both.sh(1 hunks)tests/functional/utils/run-single-test-local.sh(1 hunks)tests/functional/utils/run-single-test-remote.sh(1 hunks)tests/functional/utils/run-single-test.sh(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/functional/utils/run-single-test.sh
- tests/functional/cypress/e2e/v3/users.cy.ts
- tests/functional/cypress/e2e/v3/health.cy.ts
- tests/functional/cypress/e2e/v3/version.cy.ts
🧰 Additional context used
🧬 Code graph analysis (2)
tests/functional/cypress/e2e/v3/organization.cy.ts (1)
tests/functional/cypress/support/commands.js (4)
getAPIBaseURL(236-253)local(237-237)validate_200_Status(47-52)validate_expected_status(155-198)
tests/functional/cypress/e2e/v3/docs.cy.ts (1)
tests/functional/cypress/support/commands.js (4)
getAPIBaseURL(236-253)local(237-237)validate_200_Status(47-52)validate_expected_status(155-198)
🪛 Shellcheck (0.11.0)
tests/functional/utils/run-single-test-local.sh
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
tests/functional/utils/run-single-test-remote.sh
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
tests/functional/utils/run-single-test-both.sh
[warning] 4-4: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 8-8: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (2)
tests/functional/cypress/support/commands.js (1)
304-306: LGTM! Content-Type header is a good addition.Adding the explicit
Content-Type: application/jsonheader aligns with the pattern used ingetTokenForV3()(line 216) and ensures proper request handling by the Auth0 token API.cla-backend-go/company/handlers.go (1)
366-370: LGTM! Defensive nil-check prevents potential panic.This nil-check ensures safe dereferencing of the optional
params.CompanyNamepointer in the error logging path, preventing a potential panic if the parameter is nil.
WIPcc @mlehotskylf - previous PR had wrong base, I'll be updating this PR tomorrow.AI - skip reviews as it is a work-in-progress PR.
Signed-off-by: Lukasz Gryglicki lgryglicki@cncf.io
Assisted by OpenAI
Assisted by GitHub Copilot