Nde 95 create GitHub actions to run build script for nde customization package and addons#55
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a manual GitHub Actions build workflow for the NDE customization package and makes the existing test workflow reusable so it can gate the build.
Changes:
- Added
workflow_callsupport to the test workflow. - Added a manual build workflow that runs tests, installs dependencies, builds with a provided
view_id, and uploads the generated zip artifact.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/workflows/test.yml |
Allows the test workflow to be invoked by other workflows. |
.github/workflows/build.yml |
Adds a manual workflow for testing, building, and uploading the NDE customization package. |
Comments suppressed due to low confidence (1)
.github/workflows/build.yml:45
- The upload path duplicates the institution ID instead of deriving it from the same source as the build script.
postbuild.jsnames the zip usingINST_IDfrombuild-settings.env, so any future change to that setting would create a different file and this upload step would miss it.
path: dist/01MIT_INST-${{ inputs.view_id }}.zip
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…b-actions-to-run-build-script-for-nde-customization-package-and-addons' into NDE-95-create-github-actions-to-run-build-script-for-nde-customization-package-and-addons
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
.github/workflows/build.yml:58
- This unzipped artifact path also hard-codes the institution ID while the build output directory is named from
process.env.INST_IDinpostbuild.js(postbuild.js:7-8). Keeping the upload path independent of the build setting makes the workflow fail if the configured institution ID changes.
path: dist/01MIT_INST-${{ steps.sanitize.outputs.view_id }}/
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@cabutlermit tagging you for code review, and noting that I haven't actually tested the build workflow because apparently it needs to be on the default branch before it can be run manually. |
@adamshire123 - I noticed that you still have this as a Draft PR -- are you really ready for me to review it while it is still in "draft" mode? or are there more changes coming that I should wait for? |
ready now. |
cabutlermit
left a comment
There was a problem hiding this comment.
Let's start here with this set of requested changes (I really don't think you want to link the two workflows together).
Also, I totally feel your pain with testing new workflows -- and you are right, the .yml file must exist in the default branch before you can muck around with live testing via workflow_dispatch. When I've been in this situation in the past, I create a base workflow that doesn't really do anything other the actions/checkout and get that approved and merged into the default branch. Then, I go back and create a new feature branch where I can muck around with changes to the workflow and force manual runs in my feature branch.
| inputs: | ||
| view_id: | ||
| description: 'View ID (used to name the output zip)' | ||
| required: true | ||
| default: 'NDE' |
There was a problem hiding this comment.
It's not clear to me why you might need this as an input variable to the workflow? Will you ever be using a name other than "NDE" in the .zip file that gets generated by the workflow?
There was a problem hiding this comment.
We need to match the top level folder name in the zip file to the view ID of the Primo view to which the code is being deployed. so if I wanted to deploy the code to Primo view NDE_DEV I would need to have the folder in the zip file named NDE_DEV. the zip file can technically be named anything, but it makes sense to name it the same as the folder so you know what is in it without having to open the zip file to check.
There was a problem hiding this comment.
Ok, that makes sense why you would need this as an input for the workflow_dispatch run.
Our typical GitHub-flow works as follows:
- opening a PR from a feature branch into the default branch deploys the app into "development" environment
- merging the PR from a feature branch into the default branch deploys the app into the "staging" environment
- tagging a release on the default branch deploys the app into "production"
In that model, we typically have three workflow files -- one for "development" deploys (using the on.pull_request trigger), one for "staging" deploys (using the on.push trigger, restricted to pushes to the default branch), and one for "production" deploys (using the on.publish trigger). In that model, it seems that the workflow could have the view_id hardcoded (at least for stage & prod) since you know exactly where those files/folders are going, right?
And, one more question -- how does the npm run build know how/what to name the top level folder that eventually ends up inside the .zip file?
There was a problem hiding this comment.
npm run build uses the VIEW_ID environment variable to name that folder.
There was a problem hiding this comment.
I can't think of a good reason NOT to hard code the view_id when on.publish is triggered from the production branch. We don't really have a staging environment for Primo, so I'm not sure if it makes sense to differentiate between a push to the production branch and publishing from the production branch. I can see a world where we don't care about on.publish and just trigger a hardcoded view_id build for pushes to our mit-main branch. Happy to hear differing opinions on that, of course.
There was a problem hiding this comment.
I think we definitely want to be able to build from the feature branch without a PR from the feature branch. There are occasions where we need to build and deploy the code to a test Primo view before we're ready for a PR.
There was a problem hiding this comment.
npm run build uses the VIEW_ID environment variable to name that folder.
Sorry I'm so slow on the uptake here!
In the build-settings.env, you set the two required values (INST_ID and VIEW_ID), so if you did not do anything in this workflow file (no input, no sanitizing, no setting an environment variable for the npm run build step), wouldn't it just default to VIEW_ID=NDE from the build-settings.env file? I'm basing this on the Build the project section of the main README file.
It's not clear to me what happens if the VIEW_ID is set as an environment variable in the shell and that value is different from the value set in the build-settings.env document -- does the environment variable set in the shell override the value in the build-settings.env file? That very well might be the case, but I don't know if that's true or where that is documented.
It seems to me that that only times that you will be using the workflow_dispatch trigger would be for testing a build, not for a production deployment. Does that seem reasonable? If so, it seems that for this workflow, you simply want VIEW_ID=NDE_DEV, right?
And finally, I realized that you have both required: true and default: NDE set. I'm pretty sure that the default value will never get used since you made this a required input. If you really want it to default to NDE, you'd want required: false. Or, set required: true and don't set a default value and force the person triggering the workflow to always have to enter something.
There was a problem hiding this comment.
I can't think of a good reason NOT to hard code the view_id when on.publish is triggered from the production branch. We don't really have a staging environment for Primo, so I'm not sure if it makes sense to differentiate between a push to the production branch and publishing from the production branch. I can see a world where we don't care about on.publish and just trigger a hardcoded view_id build for pushes to our
mit-mainbranch. Happy to hear differing opinions on that, of course.
I think we definitely want to be able to build from the feature branch without a PR from the feature branch. There are occasions where we need to build and deploy the code to a test Primo view before we're ready for a PR.
You do have two different Primo environments, right? One for dev/test and one for production?
If that's true, then it seems that following makes sense:
- A
workflow_dispatchtrigger that can be run to generate a .zip build for testing (before getting to a pull request). - That same workflow can also have a build-on-pull-request-to-mit-main trigger so that you automatically get a build for Primo-test/dev when your PR is opened.
- A merge-to-mit-main trigger to automatically build the package that should be uploaded to the Primo-test/dev environment.
- A release-on-main trigger to automatically build the package that should be uploaded to the PRrmo-production environment.
The first three would all have VIEW_ID=NDE_DEV since that will always be where those build artifacts should get deployed. And the fourth would automatically have VIEW_ID=NDE since it should only get deployed to production.
There was a problem hiding this comment.
The workflow is overriding the view_id in build-settings.env with an environment variable in the shell. This is how dotenv works by default. https://github.com/motdotla/dotenv#override
I will add a comment explaining this behavior to build.yml
Yes, workflow_dispatch would only be used for testing a build, not for a production build.
In Primo we're able to create as many view codes as we need. Sometimes we need to do this to test some primo setting or some customization package in isolation. For example, last week the UX team was reviewing labels which are displaying in our NDE_DEV view (these labels are configured in Primo, not in the customization package). Simultaneously, I was working on trying to add matomo tracking code. I knew the matomo code was likely to break Primo, so rather than break the NDE_DEV view while other folks were working with it, I made a NDE_MATOMO view where I could safely test the matomo code.
All that is to say, I think it is useful to have the ability to run the build with an arbitrary view_id from a feature branch.
I believe that when you specify the workflow input as required AND specify a default, the input is just pre-filled in the UI with the default.
I'll change the default to NDE_DEV, which is the most common choice.
|
I think this is in good-enough shape at this point to merge it into the default branch so that we have a workflow there to do some testing of the I will admit that I'm still slightly confused on the "deployment" process -- you PR description talks about uploading files to Alma/Primo, but I thought that the artifacts from this build where getting published to the CDN (and that they only thing published to Alma/Primo was the URL to the CDN). |
cabutlermit
left a comment
There was a problem hiding this comment.
This is good enough for now. Once this is merged, we can run some tests of the workflow_dispatch trigger and then start planning for the next phase of automatic triggers.
There are two different ways we can add custom code to Primo. Customization Package
Add-on
|
Why these changes are being introduced
The workflows for testing and deploying an NDE customization package were complicated and required staff to have both the NDE development environment running locally in order to build the package and the necessary permissions and training to deploy customization packages to Primo via Alma admin.
Adding the ability to build the customization package via GitHub actions streamlines the testing and deployment workflows for our NDE customization package.
In the past, as part of code review, to fully test a new feature, a reviewer would need to pull down a local copy of the feature branch, install all of the dependencies, build the customization package, and finally upload it (or have someone else upload it if lacking permissions) to a dev view in Primo to see the results.
Likewise, when deploying changes to our production Primo instance the package would need to be built locally and then uploaded by someone with the correct Alma permissions.
(The NDE development environment offers some ability to test changes locally against our live systems via proxy, there are some quirks that make this less effective than uploading the built package to a Primo dev view for testing)
How this addresses that need
A new workflow file (
.github/workflows/build.yml) is introduced that:workflow_dispatch, accepting aview_idinput (used to name the output zip)view_idinput before use to prevent injection issuestest.yml), then runsnpm run builddist/01MIT_INST-<view_id>.zipand the unzipped folder) usingactions/upload-artifactNow Enterprise Systems staff can simply download the built customization package from GitHub once it is ready to upload to test or deploy to production.
Additionally,
.github/workflows/test.ymlis updated to supportworkflow_call, allowing it to be reused as a dependency of the new build workflow.Side effects of this change
The
test.ymlworkflow now also runs when called by another workflow (workflow_call), in addition to on every push. No dependency changes — the workflow relies on the existingnpm ciandnpm run buildcommands already defined in the project.Relevant ticket(s)