Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

Add needs formula #748

Merged
2 commits merged into from
Jan 10, 2022
Merged

Add needs formula #748

2 commits merged into from
Jan 10, 2022

Conversation

stathismor
Copy link
Contributor

Add NEEDS and NEEDS_ALL formula for evaluating mergeability

Change-type: minor
Signed-off-by: Stathis Moraitidis stathis@balena.io

@types/@formulajs.d.ts Outdated Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
lib/index.spec.ts Outdated Show resolved Hide resolved
lib/index.spec.ts Outdated Show resolved Hide resolved
@Hades32
Copy link
Contributor

Hades32 commented Dec 21, 2021

@stathismor
Copy link
Contributor Author

@Hades32 I pushed a commit with a new filter callback. There is something I might have misunderstood though or we didn't specify. Is the new optional function parameter, supposed to work as a filter function or should it be part of the validation? (or do we need two separate ones).

Let me elaborate. When we say, for instance NEEDS(contract, "node-module", c => c.data.architecture === "x86_64"), do we mean:
a) Check if a non-error contract has been produced for a node-module type that has an architecture of x86_64. If no, it means it's still processing, so it's pending.
b) Find a non-error backflow contract with a node-module, and check whether that has an architecture of x86_64. If no, it means it failed, therefore it's never.

We had talked about the architecture/platform case, where users will have to put multiple NEEDS to meet different platforms. So I implemented this as a) above. But I can also see the case for a usage like NEEDS(contract, "test-run", c => c.data.success === true), where the user wants to validate a specific key.

@stathismor
Copy link
Contributor Author

@Hades32 on a separate issue, I've noticed that the static-eval library we use to evaluate formulas, might have a small bug. It took me ages to figure out what's happening, but I raised is here. I doubt it will get a response soon, but let's see. Basically, even in existing code, I see that a test does:

FILTER(
    contract.links["has attached element"],
    function (c) { return c && (c.type === "message@1.0.0" || c.type === "whisper@1.0.0"); }
),

(Depending on the case) I don't think the c && should be needed, and probably it's there because of the bug. This might lead to users writing NEEDS formulas that fail without it, and have to debug. When I switched to another lib, safe-eval, it was evaluating correctly without the need of c &&.

@Hades32
Copy link
Contributor

Hades32 commented Jan 5, 2022

@stathismor about static-eval, yeah that one really sucks, it doesn't even have arrow functions...
I did look at safe-eval before and thought it would require an additional node process and discarded it, but that doesn't seem to be the case (anymore?), so I'd highly advocate for a PR to replace our eval library!

About the semantics of the callback:
You definitely made the right choice. The only reason I could come up with to want option (2) is the success check that you outlined. But I'd argue that is something is success == false then it should be an error contract.
Maybe someone will come up with another use-case, but then we'll implement it as needed

Add NEEDS and NEEDS_ALL formula for evaluating mergeability

Change-type: minor
Signed-off-by: Stathis Moraitidis <stathis@balena.io>
@stathismor stathismor force-pushed the needs-formula branch 4 times, most recently from bf70748 to 4a6524f Compare January 10, 2022 10:41
@stathismor
Copy link
Contributor Author

@Hades32 I've resolved some conflicts, had to move the enum to index.ts and restructured tests under their own function (in retrospect, I should probably have done a separate commit). Opening this as officially ready to review.

While I'm at it, I will look at the safe-eval alternative and make a PR if it works.

@stathismor stathismor marked this pull request as ready for review January 10, 2022 10:51
Copy link
Contributor

@Hades32 Hades32 left a comment

Choose a reason for hiding this comment

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

LGTM

@ghost ghost merged commit 6b7a9b2 into master Jan 10, 2022
@ghost ghost deleted the needs-formula branch January 10, 2022 13:52
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants