Do all notebook filtering in the business, not in the CI job

[fajpunk]

• This is nice because it is easier to understand and less error prone
• This stinks because a job that didn't find any notebooks to run will look just like a job that successfully ran notebooks

I'd like to mark the check run as neutral in this case, but I think it can wait until another PR.

There are a few ways to do it:

• Inspect the log output in the SolitaryResult. The more I think about this, given this notebook runner is the only case of this third type of result from a solitary execution (aside from 'success' or 'failure'), the less opposed I am to do it...
• Factor the filtering logic out and use it in both the CI worker and in the NotebookRunner. I don't love this, because:
  • it requires looking at the contents of all of the files in both places, which isn't horrible I guess
  • We'd have to remember to factor out any future logic that gets added to NotebookRunner outside of the filtering that causes a wacky situation like this
• Thread a result type through the solitary execution path and have the NotebookRunner return a SolitaryNeutralResult. This is probably the right way to do it, but it would add a lot of changes to this already-big-ish PR.

Another related thing would be putting execution logs in the github check details. I haven't done this so far because I'm worried that this would make it easy to accidentally leak some kind of sensitive info.

[rra]

I agree with doing all of the notebook filtering in the business.

I'm trying to decide if it would make sense to merge the paths between the mobu configuration and the repo configuration instead. I think that may be best? I don't like the idea that an exclusion directory in the notebook repository can cause a notebook to be included that's explicitly excluded in the Phalanx configuration.

If the configuration in the repository is invalid, I think this throws an uncaught exception that may not have all the metadata that we'd prefer. It feels like there should be a custom exception for this case that incorporates the information from the Pydantic ValidationError but also adds the normal mobu exception metadata for a better Slack exception report.

I share your worry about leaking information into the logs, like PostgreSQL passwords, but it would also be very nice to be able to see the logs. I'm anticipating the tutorial notebook case where the folks working on the notebook are going to ask us what failed. Maybe something to bring up during stand-up?

[rra]

If I'm understanding the neutral result discussion correctly, yes, I think having a solitary return a different result when it found no notebooks to execute is probably the right way to do it.

[fajpunk]

I'm trying to decide if it would make sense to merge the paths between the mobu configuration and the repo configuration instead. I think that may be best? I don't like the idea that an exclusion directory in the notebook repository can cause a notebook to be included that's explicitly excluded in the Phalanx configuration.

@rra, I think we should ditch the autostart exclude config entirely 😄 With that config in both places, any way we reconcile them is potentially confusing:

• If we override autostart with in-repo, then folks responsible for the autostart config (mostly we SQuaRE's now?) could be confused when flocks behave differently than is described in the autostart config.
• If we override in-repo with autostart, then it's confusing to folks using the GitHub CI integration when real mobu behaves differently than their CI checks did, and they might not have the knowledge or permissions to look at the autostart config.
• I think that same confusion applies if we merge the configs.

If the goal with the mobu roadmap is to promote mobu and enable more folks to use mobu to test their stuff, it seems like we should be moving stuff out of the autostart config (which only mobu admins can change) and into the payload repo. Assuming we can do it securely.

[rra]

Ah, yes, that works. Let's do it that way. It's not a meaningful security barrier since people can just put notebooks elsewhere in the repository anyway, and the problem stems from having the same information in more than one place. Eliminating either of those places will fix the problem.

[rra]

Just a couple of minor nits, otherwise looks great.