-
Notifications
You must be signed in to change notification settings - Fork 63
feat: add support for deployment "task" and multi-deployments for a given environment #410
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
Conversation
6d93085 to
e0abaad
Compare
|
👋 @xakraz hey! I see you are still actively working on this PR. I just merged a pretty large PR that migrates the project from CommonJS to ES modules and it looks like it has caused a lot of merge conflicts on your PR and just wanted to say I am sorry as that is no fun. |
Arf, I was almost there 😅 Nevermind, I will try to update my PR 🤷♂️ |
…ent auto unlock feature on pull request closure - Update the `action.yml` file to include a new `unlock_on_close_mode` input - Add a new file `unlock-on-close.js` with a function to release locks when a pull request is closed - Update the `unlock-on-merge.js` file to include a reference to `lock metadata` and other related functions - Introduce a new function `unlockOnClose` in the `main.js` file to run auto-unlock logic in the `unlock on close` mode
…ut parameter - Change description of `deployment_task` in `action.yml` - Modify parsing of task parameter in `src/functions/environment-targets.js` - Update handling of `deployment_task` in `src/functions/inputs.js` - Introduce task validation logic in `src/main.js`
ff85cc2 to
7c076ab
Compare
- Fixed ESM import conflicts in src/main.js (added .js extensions) - Updated all test files to use Vitest (vi) instead of Jest - Added missing imports in test files - Mocked actionStatus in environment-targets.test.js - Regenerated dist files with correct source maps - All tests passing with 100% coverage
7c076ab to
129fa69
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.
Pull Request Overview
This PR adds support for task-based concurrent deployments to the same environment and introduces an "unlock on close" mode. The key changes enable multiple deployment workflows to run simultaneously against the same environment by using task identifiers (e.g., --task frontend, --task backend), while maintaining separate locks per task. Additionally, it provides a new unlock-on-close mode to automatically release locks when PRs are closed (not merged).
Key Changes:
- Task parameter support for concurrent deployments with
--taskflag in PR comments - Lock mechanism enhancement to include task identifiers in branch names
- New unlock-on-close mode for automatic lock cleanup on PR closure
Reviewed Changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.js | Adds unlock-on-close mode, task validation logic, and passes task parameter through deployment flow |
| src/functions/lock.js | Updates lock branch naming to include task suffix and enhances ownership checks with branch comparison |
| src/functions/unlock.js | Adds task parameter parsing from comments and includes task in lock branch names |
| src/functions/unlock-on-merge.js | Updates to handle multiple lock branches per environment when deployment_task is 'all' |
| src/functions/unlock-on-close.js | New file implementing unlock functionality for closed (non-merged) PRs |
| src/functions/environment-targets.js | Adds task parsing from --task flag in deployment commands |
| src/functions/deployment.js | Updates deployment queries to filter by task parameter |
| src/functions/valid-deployment-order.js | Passes task parameter through to deployment checks |
| src/functions/post-deploy.js | Updates lock function calls with null task parameter |
| src/functions/inputs.js | Adds deployment_task input handling with 'all' or comma-separated list support |
| action.yml | Adds deployment_task and unlock_on_close_mode inputs, plus deployment_task output |
| tests/*.test.js | Comprehensive test coverage for all new task-related functionality |
|
Hi @GrantBirki 👋🏻 One month later, I finally got time to update my PR 😅 |
|
Hi @GrantBirki , Is there something else I should do in order to merge this PR? |
|
👋 Hey @xakraz, sorry for the delay! I was on holiday pretty much all of November and just got back today. I will try and take a look at this sometime later this week. Thanks! |
|
Thanks for the reply! |
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.
Pull request overview
Copilot reviewed 18 out of 20 changed files in this pull request and generated no new comments.
GrantBirki
left a comment
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.
@xakraz I reviewed these changes and they look phenomenal! This is a massively high quality contribution and is the reason open source makes me so happy. I'm going to merge these changes and do some extensive testing against them to ensure everything is working as expected outside of unit tests.
Once I complete testing, I'll do some maintenance updates and cut a release for you. Thanks! 🙇 ❤️
|
@xakraz I ran into a few critical issues when testing out these changes so out of an abundance of caution I rolled the changes back out. I would suggest opening a fresh PR again with these same changes and address a few of the issues and we can try to merge these changes in again. Here are some details around the issues I ran into when testing. The first issue was a pretty easy fix, I needed to add the import {unlock} from './unlock.js'
import {LOCK_METADATA} from './lock-metadata.js'
import {checkLockFile} from './check-lock-file.js'
import {checkBranch} from './lock.js'
import {constructValidBranchName} from './valid-branch-name.js'
import {COLORS} from './colors.js'I needed to do this otherwise the Action would fail on startup with an error like this: Once I got the Action to load correctly, I tried testing out a whole bunch of Here are the logs from a failed run: https://github.com/GrantBirki/actions-sandbox/actions/runs/19715041840/job/56485103776 Since Actions logs eventually expire, here they are copy/pasted for future reference: Main Step: Post Run Step: Specifically this section is of note: The expected behavior should be that if a user claims a lock for a given environment, they should be able to deploy to that environment everywhere regardless of which branch is involved. Perhaps there is a bug since It does make sense the approach that you took with making Take-away items to get this merged back in:
|
Address #168