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

Allow preconditions to be enabled via the Artifact plugin #1047

Closed
predictiple opened this issue May 2, 2021 · 1 comment
Closed

Allow preconditions to be enabled via the Artifact plugin #1047

predictiple opened this issue May 2, 2021 · 1 comment

Comments

@predictiple
Copy link
Contributor

predictiple commented May 2, 2021

Related to issue #930

The current behaviour for artifacts called by the Artifact plugin is that preconditions are disabled. This means that artifacts that are designed to be called from other artifacts have to be written and tested with this limitation in mind.

If the called artifact needs to implement conditional behaviour equivalent to multiple sources with a precondition for each source, then this has to be done using the switch() and if() functions in one block of VQL. This is far less visually comprehensible than an artifact with multiple sources with a precondition for each source, especially if viewing the artifact in a text editor that doesn't do VQL syntax highlighting.

If preconditions are only possible on top-level artifacts then this encourages the development of monolithic/standalone artifacts and discourages modular artifact development, and therefore discourages code reusability.

As an example consider this artifact that uses preconditions:

parameters:
  - name: foo
    type: bool
    default: true

sources:

  - name: A
    precondition: SELECT log(message="precondition A") FROM scope() WHERE foo
    query: SELECT "A ran" AS message FROM scope()

  - name: B
    precondition: SELECT log(message="precondition B") FROM scope() WHERE NOT foo
    query: SELECT "B ran" AS message FROM scope()

  - name: C
    precondition: SELECT log(message="precondition C") FROM scope()
    query: SELECT "C ran" AS message FROM scope()

If the above artifact needs to be called from another artifact then it has to be written thusly:

parameters:
  - name: foo
    type: bool
    default: true

sources:

  - name: AorBandC
    query: |
      SELECT * FROM chain(
        x={ SELECT * FROM switch(
            a={ SELECT *
                FROM if(condition={ SELECT log(message="precondition A") 
                                      FROM scope() WHERE foo },
                      then={ SELECT "A ran" AS message FROM scope() } ) },
            b={ SELECT *
                FROM if(condition={ SELECT log(message="precondition B") 
                                      FROM scope() WHERE NOT foo },
                      then={ SELECT "B ran" AS message FROM scope() } ) }
            )},
        c={ SELECT *
            FROM if(condition={ SELECT log(message="precondition C") 
                                  FROM scope() },
                  then={ SELECT "C ran" AS message FROM scope() } ) }
        )

The 2nd version is obviously far less comprehensible, even though it's still a relatively simple artifact (in the real world I have far more complicated artifacts) 🤮

I would like to propose that we allow preconditions to be optionally enabled when calling an artifact via the Artifact plugin. This option could be presented to the user as a new argument on the Artifact plugin, for example:
SELECT * FROM Artifact.Foo.Bar(preconditions=enabled)

The current behaviour of disabling preconditions in called artifacts could be preserved by making preconditions=disabled the default for the Artifact plugin.

@scudette
Copy link
Contributor

scudette commented Jun 9, 2021

This is now available in 0.6.0 rc1

@scudette scudette closed this as completed Jun 9, 2021
scudette added a commit that referenced this issue Apr 18, 2023
<p>This PR was automatically created by Snyk using the credentials of a
real user.</p><br /><h3>Snyk has created this PR to upgrade
moment-timezone from 0.5.41 to 0.5.42.</h3>

:information_source: Keep your dependencies up-to-date. This makes it
easier to fix existing vulnerabilities and to more quickly identify and
fix newly disclosed vulnerabilities when they affect your project.
<hr/>

- The recommended version is **1 version** ahead of your current
version.
- The recommended version was released **21 days ago**, on 2023-03-24.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>moment-timezone</b></summary>
    <ul>
      <li>
<b>0.5.42</b> - <a
href="https://snyk.io/redirect/github/moment/moment-timezone/releases/tag/0.5.42">2023-03-24</a></br><ul>
<li>Updated data to IANA TZDB <code>2023b</code></li>
</ul>
      </li>
      <li>
<b>0.5.41</b> - <a
href="https://snyk.io/redirect/github/moment/moment-timezone/releases/tag/0.5.41">2023-02-25</a></br><ul>
<li>Updated <code>moment</code> npm dependency to <code>2.29.4</code> to
remove automated warnings about insecure dependencies <a
class="issue-link js-issue-link" data-error-text="Failed to load title"
data-id="1376195089" data-permission-text="Title is private"
data-url="moment/moment-timezone#1004"
data-hovercard-type="pull_request"
data-hovercard-url="/moment/moment-timezone/pull/1004/hovercard"
href="https://snyk.io/redirect/github/moment/moment-timezone/pull/1004">#1004</a>.<br>
Moment Timezone still works with core Moment <code>2.9.0</code> and
higher.</li>
<li>Updated all dev dependencies including UglifyJS, which produces the
minified builds.</li>
<li>Added deprecation warning to the pre-built
<code>moment-timezone-with-data-2012-2022</code> bundles <a
href="https://snyk.io/redirect/github/moment/moment-timezone/issues/1035"
data-hovercard-type="issue"
data-hovercard-url="/moment/moment-timezone/issues/1035/hovercard">#1035</a>.<br>
Use the rolling <code>moment-timezone-with-data-10-year-range</code>
files instead.</li>
</ul>
      </li>
    </ul>
from <a
href="https://snyk.io/redirect/github/moment/moment-timezone/releases">moment-timezone
GitHub release notes</a>
  </details>
</details>


<details>
  <summary><b>Commit messages</b></summary>
  </br>
  <details>
    <summary>Package name: <b>moment-timezone</b></summary>
    <ul>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/81ce2526c0793454dd00f89c67531aeb30469319">81ce252</a>
Bump version in moment-timezone-utils.js</li>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/4116a04b868e63097c26a286df20e5a336e2761a">4116a04</a>
Build moment-timezone 0.5.42</li>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/c4a1ce146bb5a6600feac45732a569b1ef46e9bf">c4a1ce1</a>
changelog: Add 0.5.42</li>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/d702a49b9ce417daf17effb6ea341bc868e0b444">d702a49</a>
Bump version to 0.5.42</li>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/c008188c8271a37cfbd966a9229d21e7454fc906">c008188</a>
Merge pull request #1047 from moment/data/2023b</li>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/f094113486206d767cb1c5535444f96948d760d2">f094113</a>
tests: Fix country tests for 2023b</li>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/81e6c8132d793930c331665ee858feca68171121">81e6c81</a>
data: Add 2023b</li>
<li><a
href="https://snyk.io/redirect/github/moment/moment-timezone/commit/a8d0fa1807986d6789d1c7d4dbe3cbdef69affb1">a8d0fa1</a>
Bump json5 via npm audit fix</li>
    </ul>

<a
href="https://snyk.io/redirect/github/moment/moment-timezone/compare/98d3add7187947f37046a316802dcdfe40ad306a...81ce2526c0793454dd00f89c67531aeb30469319">Compare</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open upgrade PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiJkYzhhNzM2Ny1jNDJkLTRkOGEtOGNlMS1iNjZmMjUwNjVkMjMiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6ImRjOGE3MzY3LWM0MmQtNGQ4YS04Y2UxLWI2NmYyNTA2NWQyMyJ9fQ=="
width="0" height="0"/>

🧐 [View latest project
report](https://app.snyk.io/org/scudette/project/76f4d127-566b-42ef-86f4-bdcbc92b90b4?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR
settings](https://app.snyk.io/org/scudette/project/76f4d127-566b-42ef-86f4-bdcbc92b90b4/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade
PRs](https://app.snyk.io/org/scudette/project/76f4d127-566b-42ef-86f4-bdcbc92b90b4/settings/integration?pkg&#x3D;moment-timezone&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)

<!---
(snyk:metadata:{"prId":"dc8a7367-c42d-4d8a-8ce1-b66f25065d23","prPublicId":"dc8a7367-c42d-4d8a-8ce1-b66f25065d23","dependencies":[{"name":"moment-timezone","from":"0.5.41","to":"0.5.42"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/scudette/project/76f4d127-566b-42ef-86f4-bdcbc92b90b4?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"76f4d127-566b-42ef-86f4-bdcbc92b90b4","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":1,"publishedDate":"2023-03-24T06:33:21.236Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":false,"isBreakingChange":false,"priorityScoreList":[]})
--->

Co-authored-by: snyk-bot <[email protected]>
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

No branches or pull requests

2 participants