Deploy conductor-ready modules to modules-conductor #680
Deploy conductor-ready modules to modules-conductor #680martin-henz wants to merge 3 commits intomasterfrom
Conversation
Deploys build artifacts from the conductor-migration branch to source-academy/modules-conductor via GitHub Pages, allowing the conductor version of modules to coexist with the current deployment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout 🛎️ | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| submodules: recursive | ||
|
|
||
| - name: Enable Corepack | ||
| run: corepack enable | ||
|
|
||
| - name: Use Node.js 💻 | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version-file: .node-version | ||
| cache: yarn | ||
|
|
||
| - name: Install Dependencies 📦 | ||
| run: yarn install --immutable | ||
|
|
||
| - name: Build Modules 🔧 | ||
| run: yarn workspaces foreach -ptW --from "./src/{bundles,tabs}/*" run build | ||
|
|
||
| - name: Build Manifest | ||
| run: yarn buildtools manifest | ||
|
|
||
| - name: Build All Docs | ||
| run: yarn build:docs | ||
|
|
||
| - name: include java json | ||
| run: cp -r src/java build | ||
|
|
||
| - name: Deploy 🚀 | ||
| uses: peaceiris/actions-gh-pages@v4 | ||
| with: | ||
| deploy_key: ${{ secrets.CONDUCTOR_DEPLOY_KEY }} | ||
| external_repository: source-academy/modules-conductor | ||
| publish_dir: ./build |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| - name: include java json | ||
| run: cp -r src/java build |
There was a problem hiding this comment.
Bug: The cp -r src/java build command will cause the workflow to fail if the src/java directory does not exist, as there is no error handling.
Severity: HIGH
Suggested Fix
Wrap the cp command in a conditional check to ensure it only runs if the src/java directory exists. For example: if [ -d src/java ]; then cp -r src/java build; fi. This prevents the step from failing if the directory is absent.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: .github/workflows/conductor-deploy.yml#L38-L39
Potential issue: The `conductor-deploy.yml` workflow includes the command `cp -r
src/java build`. This command will fail with a non-zero exit code if the `src/java`
directory does not exist on the branch where the workflow is triggered. Since GitHub
Actions treats non-zero exit codes as step failures by default and there is no
conditional check or `continue-on-error` flag, the entire deployment workflow will halt
and fail. The `src/java` directory does not currently exist in the repository's main
branch, making this failure a realistic scenario.
Did we get this right? 👍 / 👎 to inform future reviews.
martin-henz
changed the title
martin-henz
changed the title
RichDom2185
left a comment
RichDom2185
left a comment
There was a problem hiding this comment.
I don't get the purpose of this PR, is it for CD? What's left to get it out of draft status?
Every push to this branch updates the modules-conductor repo. So yea, it's for CD in that sense. Having a PR allows us to easily merge any commits to the main branch into this branch, to keep modules-conductor up-to-date. |
I agree, but this sounds like this is a PR for the development trunk instead of CD then. In which case it would remain in draft forever and the workflow never get merged or run? |
3 participants
Deploys build artifacts from the conductor-migration branch to source-academy/modules-conductor via GitHub Pages, allowing the conductor version of modules to coexist with the current deployment.