Feat/ci pipelines#3
Merged
Merged
Conversation
Reviewer's GuideIntroduces a new CI integration test job, refines static code analysis workflow to handle unit and coverage artifacts, extends Docker Compose and pytest configuration for integration testing, updates dependencies, and adds a full suite of integration tests with fixtures. Class diagram for integration test fixtures and structureclassDiagram
class conftest_py {
<<fixture>>
}
class test_tickets_registration_py {
<<integration test>>
}
test_tickets_registration_py ..> conftest_py : uses fixture
Flow diagram for updated pytest test discoveryflowchart TD
A["pytest.ini"] --> B["testpaths"]
B --> C["tests/unit"]
B --> D["tests/integration"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload. (link)
General comments:
- Remove or enable the commented-out SonarQube job in pr-validations.yml to avoid confusion or stale code.
- Replace the fixed
sleep 10in the integration test job with a proper health-check or wait-for script to ensure the database is ready before running tests. - Add a teardown step after integration tests to bring down the Docker Compose services and avoid resource leaks on the runner.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Remove or enable the commented-out SonarQube job in pr-validations.yml to avoid confusion or stale code.
- Replace the fixed `sleep 10` in the integration test job with a proper health-check or wait-for script to ensure the database is ready before running tests.
- Add a teardown step after integration tests to bring down the Docker Compose services and avoid resource leaks on the runner.
## Individual Comments
### Comment 1
<location> `docker/docker-compose.yml:22-27` </location>
<code_context>
build:
context: .. # root folder
dockerfile: docker/Dockerfile.register-ticket-api
+ environment:
+ DB_HOST: ${DB_HOST}
+ DB_PORT: ${DB_PORT}
+ DB_USER: ${DB_USER}
+ DB_PASSWORD: ${DB_PASSWORD}
+ DB_NAME: ${DB_NAME}
ports:
- "8000:8000"
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Passing environment variables directly from host may expose sensitive data.
Use secure methods like Docker secrets or a vault to manage sensitive environment variables, and avoid exposing them in logs or version control.
Suggested implementation:
```
environment:
DB_HOST: ${DB_HOST}
DB_PORT: ${DB_PORT}
DB_NAME: ${DB_NAME}
secrets:
- db_user
- db_password
```
```
ports:
- "8000:8000"
depends_on:
```
```
secrets:
db_user:
file: ./secrets/db_user.txt
db_password:
file: ./secrets/db_password.txt
```
1. You must create the files `docker/secrets/db_user.txt` and `docker/secrets/db_password.txt` containing the respective secret values.
2. In your application, read the DB_USER and DB_PASSWORD from `/run/secrets/db_user` and `/run/secrets/db_password` instead of environment variables.
3. Optionally, you can also move DB_HOST, DB_PORT, and DB_NAME to secrets if they are sensitive.
4. Make sure `.gitignore` includes the `secrets/` directory to avoid committing secrets to version control.
</issue_to_address>
### Comment 2
<location> `tests/integration/test_tickets_registration.py:80` </location>
<code_context>
+ assert second_response.status_code == ERROR_STATUS_CODE
+
+
+def test_invalid_ticket_registration_error(base_url: str, test_username: str = "spuertaf") -> None:
+ headers: dict[str, str] = {"Content-Type": "application/json"}
+ payload: dict[str, str] = {"seat": "B05", "gate": "G12"}
</code_context>
<issue_to_address>
**suggestion (testing):** Clarify what makes the ticket registration invalid in this test.
Add a comment or assertion explaining why 'seat': 'B05' and 'gate': 'G12' are invalid, or include more explicit invalid cases to clarify the business rule being tested.
</issue_to_address>
### Comment 3
<location> `tests/integration/conftest.py:9-19` </location>
<code_context>
+from psycopg2._psycopg import connection
+
+
+@pytest.fixture(scope="session")
+def db_connection() -> Generator:
+ conn: connection = psycopg2.connect(
+ dbname=os.getenv("DB_NAME", "event_access"),
</code_context>
<issue_to_address>
**suggestion (testing):** Consider using a transaction rollback for database fixture to isolate tests.
Using a per-test transaction with rollback will prevent data leakage between tests and improve reliability.
```suggestion
@pytest.fixture(scope="function")
def db_connection() -> Generator:
conn: connection = psycopg2.connect(
dbname=os.getenv("DB_NAME", "event_access"),
user=os.getenv("DB_USER", "test"),
password=os.getenv("DB_PASSWORD", "TEST"),
host=os.getenv("DB_HOST", "localhost"),
port=os.getenv("DB_PORT", "5432"),
)
# Start a transaction
conn.autocommit = False
try:
yield conn
finally:
# Rollback any changes made during the test
conn.rollback()
conn.close()
```
</issue_to_address>
### Comment 4
<location> `.github/workflows/pr-validations.yml:42` </location>
<code_context>
uses: astral-sh/setup-uv@v5
</code_context>
<issue_to_address>
**security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha):** An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location> `tests/integration/test_tickets_registration.py:54` </location>
<code_context>
def test_register_ticket_flow(
base_url: str, db_connection: connection, test_username: str = "spuertaf"
) -> None:
"""
Tests whole ticket registration flow works as expected.
"""
headers: dict[str, str] = {"Content-Type": "application/json"}
payload: dict[str, str] = {"seat": "A12", "gate": "G1"}
endpoint: str = f"/api/users/{test_username}/tickets"
response = requests.post(
url=base_url + endpoint, headers=headers, json=payload, timeout=REQUEST_TIMEOUT
)
assert response.status_code == OK_STATUS_CODE
ticket_id: str = response.json().get("id")
assert ticket_id is not None
response_user_id: str = response.json().get("user_id")
assert response_user_id is not None
used_at = response.json().get("used_at")
assert used_at is None
# see if database registered the ticket
with db_connection.cursor() as cursor:
cursor.execute(
"""
SELECT
seat, gate, user_id, status
FROM tickets
WHERE
ticket_id = %s
""",
(ticket_id,),
)
result: tuple[Any, Any, Any, Any] = cursor.fetchone()
assert result is not None
seat, gate, user_id, status = result
assert seat == payload["seat"]
assert gate == payload["gate"]
assert str(user_id) == str(response_user_id)
assert status == "valid"
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove unnecessary casts to int, str, float or bool ([`remove-unnecessary-cast`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-cast/))
```suggestion
assert str(user_id) == response_user_id
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+22
to
+27
| environment: | ||
| DB_HOST: ${DB_HOST} | ||
| DB_PORT: ${DB_PORT} | ||
| DB_USER: ${DB_USER} | ||
| DB_PASSWORD: ${DB_PASSWORD} | ||
| DB_NAME: ${DB_NAME} |
There was a problem hiding this comment.
🚨 suggestion (security): Passing environment variables directly from host may expose sensitive data.
Use secure methods like Docker secrets or a vault to manage sensitive environment variables, and avoid exposing them in logs or version control.
Suggested implementation:
environment:
DB_HOST: ${DB_HOST}
DB_PORT: ${DB_PORT}
DB_NAME: ${DB_NAME}
secrets:
- db_user
- db_password
ports:
- "8000:8000"
depends_on:
secrets:
db_user:
file: ./secrets/db_user.txt
db_password:
file: ./secrets/db_password.txt
- You must create the files
docker/secrets/db_user.txtanddocker/secrets/db_password.txtcontaining the respective secret values. - In your application, read the DB_USER and DB_PASSWORD from
/run/secrets/db_userand/run/secrets/db_passwordinstead of environment variables. - Optionally, you can also move DB_HOST, DB_PORT, and DB_NAME to secrets if they are sensitive.
- Make sure
.gitignoreincludes thesecrets/directory to avoid committing secrets to version control.
Comment on lines
+9
to
+19
| @pytest.fixture(scope="session") | ||
| def db_connection() -> Generator: | ||
| conn: connection = psycopg2.connect( | ||
| dbname=os.getenv("DB_NAME", "event_access"), | ||
| user=os.getenv("DB_USER", "test"), | ||
| password=os.getenv("DB_PASSWORD", "TEST"), | ||
| host=os.getenv("DB_HOST", "localhost"), | ||
| port=os.getenv("DB_PORT", "5432"), | ||
| ) | ||
| yield conn | ||
| conn.close() |
There was a problem hiding this comment.
suggestion (testing): Consider using a transaction rollback for database fixture to isolate tests.
Using a per-test transaction with rollback will prevent data leakage between tests and improve reliability.
Suggested change
| @pytest.fixture(scope="session") | |
| def db_connection() -> Generator: | |
| conn: connection = psycopg2.connect( | |
| dbname=os.getenv("DB_NAME", "event_access"), | |
| user=os.getenv("DB_USER", "test"), | |
| password=os.getenv("DB_PASSWORD", "TEST"), | |
| host=os.getenv("DB_HOST", "localhost"), | |
| port=os.getenv("DB_PORT", "5432"), | |
| ) | |
| yield conn | |
| conn.close() | |
| @pytest.fixture(scope="function") | |
| def db_connection() -> Generator: | |
| conn: connection = psycopg2.connect( | |
| dbname=os.getenv("DB_NAME", "event_access"), | |
| user=os.getenv("DB_USER", "test"), | |
| password=os.getenv("DB_PASSWORD", "TEST"), | |
| host=os.getenv("DB_HOST", "localhost"), | |
| port=os.getenv("DB_PORT", "5432"), | |
| ) | |
| # Start a transaction | |
| conn.autocommit = False | |
| try: | |
| yield conn | |
| finally: | |
| # Rollback any changes made during the test | |
| conn.rollback() | |
| conn.close() |
| seat, gate, user_id, status = result | ||
| assert seat == payload["seat"] | ||
| assert gate == payload["gate"] | ||
| assert str(user_id) == str(response_user_id) |
There was a problem hiding this comment.
suggestion (code-quality): Remove unnecessary casts to int, str, float or bool (remove-unnecessary-cast)
Suggested change
| assert str(user_id) == str(response_user_id) | |
| assert str(user_id) == response_user_id |
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Log in Github container registry | ||
| uses: docker/login-action@v3 |
There was a problem hiding this comment.
security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha): An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
Source: opengrep
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Build and push Docker image to GHCR | ||
| uses: docker/build-push-action@v5 |
There was a problem hiding this comment.
security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha): An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
Source: opengrep
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v5 |
There was a problem hiding this comment.
security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha): An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
Source: opengrep
| uv pip install -r requirements.txt | ||
|
|
||
| - name: Load env vars from .env.test file | ||
| uses: xom9ikk/dotenv@v2 |
There was a problem hiding this comment.
security (yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha): An action sourced from a third-party repository on GitHub is not pinned to a full length commit SHA. Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps mitigate the risk of a bad actor adding a backdoor to the action's repository, as they would need to generate a SHA-1 collision for a valid Git object payload.
Source: opengrep
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Configure CI workflows to include unit and integration testing with coverage reporting and add a suite of end-to-end tests for ticket registration, including test fixtures and environment setup.
Enhancements:
CI:
Tests: