Skip to content

Conversation

alinabuzachis
Copy link
Contributor

@alinabuzachis alinabuzachis commented Jul 2, 2025

@alinabuzachis alinabuzachis marked this pull request as draft July 2, 2025 10:51
@alinabuzachis alinabuzachis force-pushed the async_create_pattern branch 4 times, most recently from c67dc6e to 35c0d3c Compare July 11, 2025 09:18
@GomathiselviS GomathiselviS marked this pull request as ready for review July 14, 2025 04:07
@alinabuzachis alinabuzachis changed the title AAP-47753: Async create() for PatternViewSet AAP-47753: Implement run_pattern_task() for PatternViewSet Jul 23, 2025
@alinabuzachis alinabuzachis force-pushed the async_create_pattern branch 3 times, most recently from 0f43195 to c8fd9b9 Compare July 24, 2025 12:27
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
4 Security Hotspots

See analysis details on SonarQube Cloud

@alinabuzachis alinabuzachis force-pushed the async_create_pattern branch 2 times, most recently from b3d8028 to bd95fbf Compare July 29, 2025 15:10
Copy link
Collaborator

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

The documentation needs to be updated to include the new required and optional env variables used to connect to an AAP/aap-dev instance.

@alinabuzachis alinabuzachis force-pushed the async_create_pattern branch 2 times, most recently from bdfb8dc to 4cc39a9 Compare August 4, 2025 15:36
@alinabuzachis alinabuzachis requested a review from hakbailey August 4, 2025 15:54
)


def _validate_url(url: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this file here? It seems that it is used to validate the settings.AAP_URL, which can be part of a utils directory and called in the ready() function of the App config

Copy link
Contributor Author

@alinabuzachis alinabuzachis Aug 6, 2025

Choose a reason for hiding this comment

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

@abikouo Can you please let me know if the changes meet your expectations? Thanks.

core/utils.py Outdated
from typing import Iterator
from urllib.parse import urljoin

from pattern_service.settings.aap import AAP_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

You should import the django.conf.settings instead

from requests import Session
from requests.auth import HTTPBasicAuth

from pattern_service.settings.aap import AAP_PASSWORD
Copy link
Contributor

Choose a reason for hiding this comment

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

The settings are directly available as django.conf import settings

Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis alinabuzachis requested a review from abikouo August 6, 2025 12:17
Copy link
Contributor

@abikouo abikouo left a comment

Choose a reason for hiding this comment

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

Looks good overall, but still not getting the point why we need the settings/aap.py file

@alinabuzachis
Copy link
Contributor Author

Looks good overall, but still not getting the point why we need the settings/aap.py file

The idea behind this file was to manage the authentication credentials for AAP needed to download the collection from the hub instance. I'm open to suggestions, so feel free to propose something else.

@abikouo
Copy link
Contributor

abikouo commented Aug 6, 2025

Looks good overall, but still not getting the point why we need the settings/aap.py file

The idea behind this file was to manage the authentication credentials for AAP needed to download the collection from the hub instance. I'm open to suggestions, so feel free to propose something else.

I understand the point, but currently, the file aap.py has no effect since it is not imported anywhere.
The fact that you are calling settings.AAP_URL, means the app expects the AAP_URL to be defined somewhere in the settings. You can just ensure that the required AAP variables are defined in the apps ready() function.
In the settings file, we are already doing a dynamic configuration, which means when deploying the app, you just need to define the env variable PATTERN_SERVICE_AAP_URL (same for others required variables) and this will be turned to AAP_URL into the settings.

Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis
Copy link
Contributor Author

I understand the point, but currently, the file aap.py has no effect since it is not imported anywhere. The fact that you are calling settings.AAP_URL, means the app expects the AAP_URL to be defined somewhere in the settings. You can just ensure that the required AAP variables are defined in the apps ready() function. In the settings file, we are already doing a dynamic configuration, which means when deploying the app, you just need to define the env variable PATTERN_SERVICE_AAP_URL (same for others required variables) and this will be turned to AAP_URL into the settings.

Ok, right! I remove the app.py file and added the required check inside the ready(). Let me know please if anything else needs to be updated.

Args:
pattern_id (int): The ID of the pattern to process.
task_id (int): The ID of the task..
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
task_id (int): The ID of the task..
task_id (int): The ID of the task.

Copy link
Contributor

@beeankha beeankha left a comment

Choose a reason for hiding this comment

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

Just one super minor edit

Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Copy link
Contributor

@GomathiselviS GomathiselviS left a comment

Choose a reason for hiding this comment

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

Just a couple of minor comments, otherwise this LGTM.

logger.error(f"Could not find pattern definition for task {task_id}")
task.mark_failed({"error": "Pattern definition not found."})
except Exception as e:
logger.error(f"Task {task_id} failed: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use logger.exception to include the full traceback in logs for debugging?

except Exception:
      error_message = "An unexpected error occurred."
      logger.exception(f"Task {task_id} failed unexpectedly.")
      task.mark_failed({"error": error_message})

temporary directory.
Args:
collection: The name of the collection (e.g., 'my_namespace.my_collection').
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
collection: The name of the collection (e.g., 'my_namespace.my_collection').
collection_name: The name of the collection (e.g., 'my_namespace.my_collection').

Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
Copy link
Collaborator

@hakbailey hakbailey left a comment

Choose a reason for hiding this comment

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

Just a few more small changes.

Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis alinabuzachis merged commit 3348a51 into ansible:main Aug 13, 2025
4 checks passed
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.

5 participants