Open cticenhour opened 1 year ago
FYI - I've now got a set of test recipes queued up for CRANE whenever we get the rest of the setup complete. Not checking off those items until they're formally merged into CIVET.
I merged the pull request after testing the branch on my machine, including the git submodule update --init moose. Seems to be some checks failing with it, though. See my comments on #87. I updated the README to include 'git submodule update --init moose' step within the installation instructions.
CRANE has been added to the MOOSE external apps list and a set of test configurations have been added to CIVET. The next step here is to add the webhook (so that GitHub can interface CIVET with CRANE). If you go to the Settings for the repository, you'll see Webhooks on the left hand side. Clicking that should give an empty list and you'll see a button on the right to add a webhook. I will email you individually @smpeyres to give you the configuration needed for CIVET. Stay tuned!
@smpeyres Checking in here - was the webhook for CIVET event communication added to CRANE settings? If so, I think testing it with a MOOSE submodule update will allow you to close this issue.
@cticenhour No, I havent been unable to add the webhook. I don't see the option within the settings--I wonder if there is a moderator privilege I don't have? @dcurreli
I sent an email regarding this on Dec 15th, btw
Making a comment here and tagging @dcurreli and @cticenhour - need to prioritize this effort again! The git submodule update --init moose
step is causing failed compilations (see #100 ; I've also been personally asked about this error). Need to make sure CRANE is always instep with MOOSE and to get MOOSE-team help with fixes when needed.
For now, I want to remove the git submodule update --init moose
step until continuous integration is operational. See pull request #101
In order to get CI fully enabled we need the webhook in place - @dcurreli would be the one to do that. I can re-send the email instructions I gave on this if necessary.
CRANE is guaranteed to work with the most current version of MOOSE (as of today) due to Zapdos being continuously updated, so a manual update of the submodule could be done to avoid this in future (without changing the instructions). @smpeyres do you know how to do that update? Before I had automatic updates set up for Zapdos, I would update the submodule manually to MOOSE master branch every few weeks.
@cticenhour I do not know how to manually update the moose submodule...
No worries! It's really easy. You want to do the following (assuming crane is in your ~/projects
folder):
cd ~/projects/crane
git submodule update --init moose
cd moose
git fetch origin
git checkout origin/master
cd ..
git checkout -b moose-update
git add moose
git commit -m "Update moose submodule"
git push origin moose-update
And then you make a PR to merge the moose-update
branch.
Hi Casey, @dcurreli just made a dummy PR #112 after creating a webhook following your instructions from an email back in Dec 2022. We now see a crane tab on the civet.inl.gov website, https://civet.inl.gov/repo/821/.
What are the next steps?
Thanks for getting the initial webhook set up. I can see that the PR and the merge triggered two events, but that the Precheck failed for both for the following reasons:
The PR was being made against the master
branch directly instead of the expected workflow through a devel
branch. See my comment from above:
Another change I would make would be the creation of a devel branch. After a PR merge, everything would be re-tested and then merged into master only when everything is passing. The branch testing would be where you might have extended testing in the future.....at first it might have the same test configurations - we call them "recipes" - as your PRs. This workflow protects your master branch from possible failure points that might occur when multiple people are working on the code. Having a devel branch is optional, but all of the INL MOOSE apps and several external apps use this structure.
The master branch precheck failed because we have some style checking turned on for trailing whitespaces. Once the above failure is addressed, you should be able to see this in a PR precheck to fix it.
@smpeyres and @dcurreli - the question for you both now is whether you want to add a devel branch and protect the current master against individual pushes. If so, I can guide on what settings are required. If not, we can adjust the CI settings to allow for your current approach.
Base branch changed from master
to devel
.
Thank you for the update @dcurreli. I will work on adding some recipes to CIVET to enable the "devel to master" merge after successful completion of devel testing, and double-check for anything else that might be required. Likely will set up another testing PR myself to make sure the whole system is working as expected at that point.
@dcurreli With the merge of #115, testing passed, but the merge to master did not. GitHub CLI reported the following issue:
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: Commits must have valid signatures. Changes must be made through a pull request.
To github.com:lcpp-org/crane
! [remote rejected] master -> master (protected branch hook declined)
error: failed to push some refs to 'github.com:lcpp-org/crane'
Looks like the signature verification and PR requirement for master
needs to be relaxed, now that PR merges occur on devel
. If you still want this for PRs on CRANE, you can move those requirements to devel
.
@dcurreli With the merge of #115, testing passed, but the merge to master did not. GitHub CLI reported the following issue:
remote: error: GH006: Protected branch update failed for refs/heads/master. remote: error: Commits must have valid signatures. Changes must be made through a pull request. To github.com:lcpp-org/crane ! [remote rejected] master -> master (protected branch hook declined) error: failed to push some refs to 'github.com:lcpp-org/crane'
Looks like the signature verification and PR requirement for
master
needs to be relaxed, now that PR merges occur ondevel
. If you still want this for PRs on CRANE, you can move those requirements todevel
.
@cticenhour We just switched the signature verification from master to devel. Let us know if this helps fixes the automatic merge to master.
@smpeyres We'll see shortly! Stay tuned here: https://civet.inl.gov/job/1840148/
Since you only mentioned signature verification, I foresee an issue if the PR-only merge requirement is still on for master. That should be a devel branch protection item, with master having moosebuild
as the only allowed external "push" user. See here for an example of that in the branch protection settings:
@smpeyres @dcurreli Looks like the branch merge worked well with your current settings. The basic CI workflow is now done, with "enhancements" - automatic integration into the PR status, notably, without needing to manually peek at CIVET - being available, pending discussion with our CI technicians regarding permissions.
So, this PR could be tentatively closed, and re-opened as things develop re: the PR integration and more features are required.
INL's continuous integration (CI) system CIVET allows for testing of external MOOSE applications like CRANE, both within their own repository (PRs, branch merges, etc.) and as part of regular MOOSE PR testing - see this page for a recent set of results on the MOOSE
next
branch. CRANE could benefit from this workflow, given as up until now CRANE components have been tested only locally and as part of their usage in Zapdos.Another change I would make would be the creation of a
devel
branch. After a PR merge, everything would be re-tested and then merged intomaster
only when everything is passing. The branch testing would be where you might have extended testing in the future.....at first it might have the same test configurations - we call them "recipes" - as your PRs. This workflow protects yourmaster
branch from possible failure points that might occur when multiple people are working on the code. Having adevel
branch is optional, but all of the INL MOOSE apps and several external apps use this structure.To get this rolling, we would need to perform a few things:
Happy to answer any questions you might have on this!
Tagging @smpeyres @dcurreli