-
Notifications
You must be signed in to change notification settings - Fork 1
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
DM-44635: mobu in CI #352
DM-44635: mobu in CI #352
Conversation
17002bf
to
e10c700
Compare
71fb9ad
to
123f3a0
Compare
b20e9ba
to
7e47bb8
Compare
7e47bb8
to
6a29989
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have a bunch of minor comments and only a few things that jumped out at me.
For the future (no need to break this up), this is a lot for one PR. Ideally this would be three or four PRs (the documentation build system and new manual as a separate PR, for instance), which would make it easier to review, if only because it would be easier to do one PR, take a break, come back and do another, etc. I know it's really hard to break up large PRs like this when you want to test the combination of them all.
Usually what I do is maintain a branch with a whole queue of commits, test them all together, and then cherry-pick them into separate branches to submit as PRs. That way the reviewer can work through the stack while I'm continuing development and neither blocks on the other.
|
||
Scenarios: | ||
|
||
### Job completes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Once we start generating Python API docs, which I usually do even for applications when they have a doc site since it can be very useful for development, it will want all of this to be reformatted as rST, not Markdown.
for job in self._jobs: | ||
await job.wait() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future work here too: we should probably just shut down the notebook immediately rather than wait for the jobs to finish.
3d15c64
to
03cd564
Compare
fcbae04
to
69e0c77
Compare
1a38681
to
b6302b7
Compare
b6302b7
to
33b383f
Compare
Mobu in CI
docs/
in this PR) for more detailed info on how this works from both user and admin perspectivesThere is a separate GitHub app for each environment for each of the refresh and CI functionalities. This lets you enable these integrations for different combinations of environments. For example, you can enable the auto-refresh integration in
idfdev
,idfint
,usdfdev
,usdfint
, andusdfprod
, but the CI integration only inidfint
andusdfint
.Goes with the Phalanx changes in lsst-sqre/phalanx#3453
Testing
Test repo
This repo has a bunch of different notebooks in a bunch of different directories, some of which intentionally throw exceptions.
It contains a script to update certain notebooks, conditionally updating the poison ones.
I installed the data-dev Mobu CI app in the lsst-sqre GitHub org, and configured it to access "Only select repositories", and I added the
dfuchs-test-mobu
repo in the dropdown.Local testing with smee.io
I installed the tool from smee.io and pointed it at my running local mobu:
I configured the data-dev GitHub Mobu CI app to send webooks to the smee URL.
I ran my local mobu against
data-dev
:The requests sent by the smee proxy have
:port
suffixes in theX-Forwarded-For
values. Safir doesn't handle this (and I don't think it's Safir's fault; I think the port should be inX-Forwarded-Port
), so I ran a local safir:With this change:
data-dev testing
Queued
In Progress
Success
Failure
Failure due to mobu stop/restart
Success re-running check
Success re-running suite