Skip to content

AAP-47731: create run_pattern_instance_task for PatternIstanceViewSet #16

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 20, 2025
Merged

AAP-47731: create run_pattern_instance_task for PatternIstanceViewSet #16

merged 1 commit into from
Aug 20, 2025

Conversation

alinabuzachis
Copy link
Contributor

@alinabuzachis alinabuzachis commented Jul 2, 2025 •
edited
Loading

@alinabuzachis alinabuzachis changed the title AAP-47731: Async create() PatternIstanceViewSet AAP-47731: create run_pattern_instance_task for PatternIstanceViewSet Jul 24, 2025
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
37.7% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch from 57ca03b to 1e063c4 Compare July 25, 2025 11:10
@alinabuzachis alinabuzachis marked this pull request as ready for review July 28, 2025 13:33
@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch 2 times, most recently from 6606173 to 122c7d1 Compare July 29, 2025 18:38
GomathiselviS
GomathiselviS reviewed Aug 1, 2025
View reviewed changes
core/services.py Outdated
Downloads and extracts a collection tarball to a 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: The name of the collection (e.g., 'my_namespace-my_collection').

core/tests/test_services.py
)
mock_update_status.assert_any_call(
task, "Completed", {"info": "Pattern processed successfully"}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

test_full_status_update_flow test already covers everything this test does and more. Should we merge both the tests?

core/tests/test_services.py Outdated
pattern_name="test_pattern",
)
task = Task.objects.create(status="Initiated", details={})
temp_dir = tempfile.mkdtemp()
Copy link
Contributor

Choose a reason for hiding this comment

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

SharedDataMixin already has a helper, create_temp_collection_dir, that does this. SHould we use that here?
temp_dir= self.create_temp_collection_dir()

core/tests/test_views.py Outdated
response = self.client.post(url, data, format="json")
@patch("core.views.run_pattern_instance_task")
def test_create_pattern_instance_and_task(self, mock_run_task):
request = self.factory.post(
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 self.client here to be consistent with the other tests?

@alinabuzachis alinabuzachis mentioned this pull request Aug 4, 2025
Merged
@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch 3 times, most recently from f43618a to 27b74eb Compare August 7, 2025 08:53
@alinabuzachis
Copy link
Contributor Author

@GomathiselviS Thank you for reviewing. Can you please a look at #15 since that one will need to be merged first. I have already applied some of your suggestions to #15, so I'll appreciate your review on that PR too. Thanks.

@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch 2 times, most recently from 0fe6b20 to 56040b6 Compare August 13, 2025 19:06
beeankha
beeankha reviewed Aug 14, 2025
View reviewed changes
core/utils/controller/client.py Outdated
Comment on lines 91 to 95
if results:
logger.debug(
f"""Resource already exists. Returning
existing resource: {results[0]}"""
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Had to alter line length here to make the linter happy, which led to a slightly more confusing-looking diff

GomathiselviS
GomathiselviS reviewed Aug 14, 2025
View reviewed changes
core/utils/controller/helpers.py Outdated

role_id = roles_resp["results"][0]["id"]

for auto in automations:
Copy link
Contributor

Choose a reason for hiding this comment

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

The function iterates through each job template and then through each user and team, making a separate POST API call for every single role assignment. Can this be refactored to perform role assignments in bulk?

Copy link
Contributor Author

@alinabuzachis alinabuzachis Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I do not see the API supporting bulk operations for role assignment. https://ansible.readthedocs.io/projects/awx/en/latest/rest_api/api_ref.html#/Bulk https://ansible.readthedocs.io/projects/awx/en/24.6.1/userguide/rbac.html

GomathiselviS
GomathiselviS approved these changes Aug 15, 2025
View reviewed changes
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.

LGTM

hakbailey
hakbailey reviewed Aug 15, 2025
View reviewed changes
@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch 3 times, most recently from 612843e to f70ac1d Compare August 18, 2025 15:58
@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch 2 times, most recently from 2640e93 to 5869392 Compare August 19, 2025 14:55
hakbailey
hakbailey approved these changes Aug 20, 2025
View reviewed changes
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.

:shipit:

@hakbailey
Copy link
Collaborator

@alinabuzachis will you squash the commits before you merge this please? Thank you!

@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch from 9dc3d52 to e6505e7 Compare August 20, 2025 14:03
@alinabuzachis
Add run_pattern_instance_task
c525965 
Signed-off-by: Alina Buzachis <[email protected]>
@alinabuzachis alinabuzachis force-pushed the async_create_pattern_instance branch from e6505e7 to c525965 Compare August 20, 2025 14:04
@alinabuzachis alinabuzachis merged commit ca70f2c into ansible:main Aug 20, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beeankha beeankha beeankha left review comments

@hakbailey hakbailey hakbailey approved these changes

@GomathiselviS GomathiselviS GomathiselviS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alinabuzachis @hakbailey @beeankha @GomathiselviS