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

Add a CI workflow to CRANE #86

Open
4 tasks done
cticenhour opened this issue Dec 6, 2022 · 18 comments
Open
4 tasks done

Add a CI workflow to CRANE #86

cticenhour opened this issue Dec 6, 2022 · 18 comments
Assignees

Comments

@cticenhour
Copy link
Collaborator

cticenhour commented Dec 6, 2022

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 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.

To get this rolling, we would need to perform a few things:

  • Add a MOOSE submodule to CRANE (I have a branch ready to submit for this shortly).
  • Add CRANE to the list of tested external applications (I would perform this internally).
  • Add a MOOSE webhook that allows GitHub to communicate with CIVET when a new event (like a PR or branch merge) occurs.
  • Add a set of regular CRANE test configurations for PRs and branches. Again, I would do this internally and be your point of contact if you wanted to make changes. At first, I think we should base this on those found in Zapdos.

Happy to answer any questions you might have on this!

Tagging @smpeyres @dcurreli

@smpeyres smpeyres self-assigned this Dec 6, 2022
@cticenhour
Copy link
Collaborator Author

cticenhour commented Dec 6, 2022

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.

smpeyres added a commit that referenced this issue Dec 9, 2022
Added command to update and initialize the moose submodule. See commit #87 and issue #86
@smpeyres
Copy link
Collaborator

smpeyres commented Dec 9, 2022

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.

@cticenhour
Copy link
Collaborator Author

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!

@cticenhour
Copy link
Collaborator Author

@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.

@smpeyres
Copy link
Collaborator

@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

@smpeyres smpeyres mentioned this issue May 9, 2023
@smpeyres
Copy link
Collaborator

smpeyres commented May 9, 2023

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

@cticenhour
Copy link
Collaborator Author

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.

@smpeyres
Copy link
Collaborator

smpeyres commented May 9, 2023

@cticenhour I do not know how to manually update the moose submodule...

@cticenhour
Copy link
Collaborator Author

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.

@smpeyres
Copy link
Collaborator

smpeyres commented Sep 5, 2023

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?

@cticenhour
Copy link
Collaborator Author

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.

@dcurreli
Copy link
Contributor

Base branch changed from master to devel.

@cticenhour
Copy link
Collaborator Author

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.

cticenhour added a commit to cticenhour/crane that referenced this issue Sep 12, 2023
cticenhour added a commit to cticenhour/crane that referenced this issue Sep 12, 2023
cticenhour added a commit to cticenhour/crane that referenced this issue Sep 12, 2023
Where part of features, transition to mooseError and mooseWarning where applicable. Where commented out, remove if simple diagnostics and testing and fixup where prior features exist or where well-noted developer diagnostic pathways exist.

Refs lcpp-org#86
cticenhour added a commit to cticenhour/crane that referenced this issue Sep 13, 2023
cticenhour added a commit to cticenhour/crane that referenced this issue Oct 3, 2023
@cticenhour
Copy link
Collaborator Author

@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.

@smpeyres
Copy link
Collaborator

@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.

@cticenhour We just switched the signature verification from master to devel. Let us know if this helps fixes the automatic merge to master.

@cticenhour
Copy link
Collaborator Author

@smpeyres We'll see shortly! Stay tuned here: https://civet.inl.gov/job/1840148/

@cticenhour
Copy link
Collaborator Author

cticenhour commented Oct 24, 2023

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:

image

@cticenhour
Copy link
Collaborator Author

@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.

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

3 participants