[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign grokspawn for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	91d3d29
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/684be9a5e11604000876aee9
😎 Deploy Preview	https://deploy-preview-2020--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.84%. Comparing base (efc6657) to head (91d3d29).

@@            Coverage Diff             @@
##             main    #2020      +/-   ##
==========================================
+ Coverage   73.78%   73.84%   +0.05%     
==========================================
  Files          80       80              
  Lines        7146     7146              
==========================================
+ Hits         5273     5277       +4     
+ Misses       1550     1547       -3     
+ Partials      323      322       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

should we rename the other scripts to append _script?

Hi @perdasilva

should we rename the other scripts to append _script?

I did that. If you have a better idea, please feel free to share.
But also, I think we can improve in the long run.

Is this in response to an actual problem, or a perceived future one?
The benefit of the current approach is that it prevents a PR which breaks a demo from merging. This would be delayed and possibly manual under the new regime.

Hi @grokspawn

Is this in response to an actual problem, or a perceived future one?
The benefit of the current approach is that it prevents a PR which breaks a demo from merging. This would be delayed and possibly manual under the new regime.

Today **we don’t run all demos in CI—**just the ones listed in the catalogd catalog.

We can run all of them via GitHub action in CI, but over time that might slow things down or get expensive. IMO agree it’s better to wait for real problems before adding more work. That is my preferred choice

Personally, I’d keep running demos in CI only when related code changes. They kind of act like end-to-end tests, especially for feature-gated stuff where we don’t have full test coverage yet.

We moved to a cronjob because of @perdasilva’s concerns about CI time.

If @perdasilva is okay with it, I’m happy to move the demo call back to CI—and only switch to cronjob if it becomes a problem again. That feels like the best approach to me 👍

@grokspawn yeah, I'm not in favor of running a 15 minute job (which will only grow) on a PR basis for something that seems more like stale documentation to me. Don't get me wrong, it matters. I'm just not sure we want to hold up PRs because of it, or run it with this level of frequency. We may want to hold a release back, but PRs, probably not.

The unit tests and e2e suites should already give us pretty good signal that things are working as they should. I don't think we should use demos for that. In any case, the biggest vector for demo failures would probably be changes in API surface, which should occur fairly seldomly. The failure rate of the catalogd-demo GHA is extremely low (and in the failures I've seen, it's not even exactly clear that the RC was something wrong with the demo).

Tbh, thinking a bit more about it, I'd start by making this a non-blocking step of the release pipeline and use fewer resources.

If we feel strongly about running this more frequently than once before release, maybe a good compromise would be to only run it when there are relevant manifest/API changes?

cc @camilamacedo86

should we rename the other scripts to append _script?

my bad if I missed it. I was looking in the output for the job and didn't see them (which doesn't mean they weren't there XDD) - thank you!!!

Hi @grokspawn @perdasilva

Thank you for your support and inputs 🎉

If we feel strongly about running this more frequently than once before release, maybe a good compromise would be to only run it when there are relevant manifest/API changes?

The way that we configure it here now is:

• If we change the action, add a new demo script, or modify any part of the hack/demo, we execute the action in the CI.
• Otherwise, it will run once a day as a cron job. So, if it fails, we have the opportunity to identify the latest change and fix it as soon as possible.
• Also, I add the paths when there are relevant manifest/API changes; we call it in the CI when we make changes there.

I think it is a good compromise that can make everybody happy 👍
Let me know wdyt?

/lgtm