Skip to content

ENT-10184: Top down evaluation #5834

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 4 commits into from
Aug 14, 2025
Merged

ENT-10184: Top down evaluation #5834

merged 4 commits into from
Aug 14, 2025

Conversation

victormlg
Copy link
Contributor

@victormlg victormlg commented Jul 1, 2025 •
edited
Loading

@victormlg victormlg force-pushed the top-down-evaluation branch from 51e4721 to c255477 Compare July 1, 2025 15:03
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 1, 2025
View reviewed changes
@victormlg victormlg force-pushed the top-down-evaluation branch 4 times, most recently from eccfbbf to 29b776d Compare July 2, 2025 14:13
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 2, 2025
View reviewed changes
@victormlg victormlg force-pushed the top-down-evaluation branch 4 times, most recently from a66b123 to 5038b0c Compare July 2, 2025 15:16
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 3, 2025
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@victormlg victormlg force-pushed the top-down-evaluation branch from a76ce54 to 1f0e9b7 Compare July 3, 2025 11:23
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 3, 2025
View reviewed changes
@victormlg victormlg force-pushed the top-down-evaluation branch from 1f0e9b7 to b1b9bad Compare July 3, 2025 12:10
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 3, 2025
View reviewed changes
@victormlg victormlg force-pushed the top-down-evaluation branch 4 times, most recently from 599311a to 797bed8 Compare July 3, 2025 12:45
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 3, 2025
View reviewed changes
libpromises/policy.c
@@ -1452,6 +1454,7 @@
}

SeqAppend(section->promises, pp);
SeqAppend(section->parent_bundle->all_promises, pp);

Check notice

Code scanning / CodeQL

Pointer argument is dereferenced without checking for NULL Note

Parameter section in BundleSectionAppendPromise() is dereferenced without an explicit null-check
@victormlg victormlg marked this pull request as ready for review July 3, 2025 14:59
@victormlg victormlg requested a review from olehermanse July 3, 2025 15:05
olehermanse
olehermanse reviewed Jul 4, 2025
View reviewed changes
olehermanse
olehermanse reviewed Jul 4, 2025
View reviewed changes
Copy link
Member

@olehermanse olehermanse 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 so far, please add some examples and tests so we can discuss and experiment with how it works :)

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 8, 2025
View reviewed changes
@victormlg victormlg force-pushed the top-down-evaluation branch 2 times, most recently from fd5b98b to cd37f33 Compare July 14, 2025 13:26
olehermanse
olehermanse reviewed Jul 14, 2025
View reviewed changes
olehermanse
olehermanse reviewed Jul 14, 2025
View reviewed changes
@victormlg victormlg force-pushed the top-down-evaluation branch from cd37f33 to 1dcceca Compare July 15, 2025 09:37
@victormlg
Copy link
Contributor Author

@cf-bottom jenkins, please :)

@cf-bottom
Copy link

Alright, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/12355/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12355/

larsewi
larsewi requested changes Jul 17, 2025
View reviewed changes
Copy link
Contributor

@larsewi larsewi left a comment

Choose a reason for hiding this comment

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

This is very cool @victormlg 🚀 Only some nitpicks, looking forward to try this!

@victormlg victormlg force-pushed the top-down-evaluation branch from 1dcceca to 7ec8ce5 Compare July 30, 2025 10:27
@victormlg
Copy link
Contributor Author

@cf-bottom jenkins, please :)

@cf-bottom
Copy link

Alright, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/12430/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12430/

@victormlg victormlg requested a review from olehermanse July 30, 2025 11:03
@larsewi
Copy link
Contributor

larsewi commented Aug 13, 2025

@cf-bottom Jenkins please :)

@larsewi larsewi self-requested a review August 13, 2025 13:58
@cf-bottom
Copy link

Alright, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/12477/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12477/

larsewi
larsewi requested changes Aug 13, 2025
View reviewed changes
Copy link
Contributor

@larsewi larsewi left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you move the custom promise types related tests to masterfiles/tests/acceptance/30_custom_promise_types ? Here we already have a copy of cfengine.py.

@victormlg
Copy link
Contributor Author

Can you move the custom promise types related tests to masterfiles/tests/acceptance/30_custom_promise_types ? Here we already have a copy of cfengine.py.

I don't see any copy of cfengine.py in masterfiles/tests/acceptance/30_custom_promise_types. I think it makes sense to keep the top_down evaluation tests together

@victormlg victormlg requested a review from larsewi August 14, 2025 07:51
@victormlg
Allow top-down evaluation of promises
73c445a 
Ticket: ENT-10184
Signed-off-by: Victor Moene <[email protected]>
@victormlg
Added better verbose output
3174c20 
Signed-off-by: Victor Moene <[email protected]>
@victormlg
Changed from normal to classic
f9eb0eb 
Signed-off-by: Victor Moene <[email protected]>
@victormlg victormlg force-pushed the top-down-evaluation branch from 7ec8ce5 to 57a972a Compare August 14, 2025 08:08
@larsewi
Copy link
Contributor

larsewi commented Aug 14, 2025

Can you move the custom promise types related tests to masterfiles/tests/acceptance/30_custom_promise_types ? Here we already have a copy of cfengine.py.

I don't see any copy of cfengine.py in masterfiles/tests/acceptance/30_custom_promise_types. I think it makes sense to keep the top_down evaluation tests together

It is in masterfiles/modules/promises/cfengine.py. I think it makes sense to put it in with the other custom promise types. If we release a new version of cfengine.py, we don't want to have to update it in many different repositories.

@victormlg victormlg force-pushed the top-down-evaluation branch from 57a972a to 2a7390b Compare August 14, 2025 12:57
@victormlg victormlg mentioned this pull request Aug 14, 2025
Open
@victormlg victormlg force-pushed the top-down-evaluation branch from 2a7390b to de6ce6a Compare August 14, 2025 12:59
@larsewi larsewi merged commit 1ab68a9 into cfengine:master Aug 14, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsewi larsewi larsewi approved these changes

@olehermanse olehermanse Awaiting requested review from olehermanse

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@victormlg @cf-bottom @larsewi @olehermanse