Skip to content
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.

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

AAP-10738 DROOLS-7475 Proposed feature for ['key'] accessor #539

Merged
merged 16 commits into from
Aug 3, 2023

Conversation

tarilabs
Copy link
Contributor

@tarilabs tarilabs commented Jun 14, 2023

@Alex-Izquierdo
Copy link
Contributor

I would add e2e test cases, specially to ensure the new accessor works combined with filters like selectattr, e.g: https://github.com/ansible/ansible-rulebook/blob/main/tests/e2e/files/rulebooks/operators/test_selectattr_operator.yml

@tarilabs
Copy link
Contributor Author

I would add e2e test cases, specially to ensure the new accessor works combined with filters like selectattr, e.g: https://github.com/ansible/ansible-rulebook/blob/main/tests/e2e/files/rulebooks/operators/test_selectattr_operator.yml

Done with 36ecfd6

Verified locally with:
Screenshot 2023-06-23 at 13 29 19

Naturally because of these new e2e test will now require first the merge of:

@tarilabs tarilabs marked this pull request as ready for review June 23, 2023 11:44
setup.cfg Outdated Show resolved Hide resolved
@tarilabs tarilabs changed the title DROOLS-7475 Proposed feature for ['key'] accessor AAP-10738 DROOLS-7475 Proposed feature for ['key'] accessor Jun 27, 2023
@tarilabs
Copy link
Contributor Author

@Alex-Izquierdo :

In that case can I ask for an update the of these docs for a better explanation to set properly the user expectation? Anyway this PR should include documentation to indicate the support for the new accessor.
Also, it should include it in the changelog. https://github.com/ansible/ansible-rulebook/blob/main/CHANGELOG.md

done with 68cc376

Alex-Izquierdo
Alex-Izquierdo previously approved these changes Jun 28, 2023
Copy link
Contributor

@Alex-Izquierdo Alex-Izquierdo left a comment

Choose a reason for hiding this comment

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

Minor comment regarding the changelog, otherwise LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
- avoid internal jiras notatio
- consistent terminology to doc and playbook doc
Alex-Izquierdo
Alex-Izquierdo previously approved these changes Jun 29, 2023
need to incorporate:
- kiegroup/drools-ansible-rulebook-integration#65
- ansible/drools_jpy#51

for a indexing fix to support once_after properly
@tarilabs
Copy link
Contributor Author

required bump of drools_jpy from 51e04f9 tested, in full, locally:

Screenshot 2023-07-26 at 14 45 43

@benthomasson
Copy link
Contributor

I'd like to get these tests passing before approving and merging.

@tarilabs
Copy link
Contributor Author

@benthomasson

I'd like to get these tests passing before approving and merging.

As mentioned in exchanges, the tests are failing because:

ERROR: Could not find a version that satisfies the requirement drools-jpy==0.3.6 (from ansible-rulebook) 

it needs the drools_jpy being published.

@mkanoor mkanoor merged commit bc4f8b8 into ansible:main Aug 3, 2023
6 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.

4 participants