Feature/multi channel approver - #15
bpadhan-ks wants to merge 12 commits into
Conversation
Keeper - Google chat Integration
Keeper - Google chat Integration
Publishes keeper/gchat-app to DockerHub for linux/amd64 and linux/arm64. - ci.yml builds both architectures on pull requests and runs a smoke test - docker-release.yml publishes :vX.Y.Z and :<short-sha> on a version tag - docker-promote-latest.yml moves :latest by copying an already-published digest, so :latest resolves to an image that was verified in a deployment - Taskfile provides the tag/tag-rc/promote entry points and dev helpers - Dockerfile: Node 22, and npm ci against the lockfile with no fallback - Add .dockerignore, docker-compose.example.yml, CHANGELOG.md, RELEASING.md - Fix the README Docker section, which referenced a docker-compose.yml that gchat-app-setup generates rather than one committed to the repo - Ignore the generated docker-compose.yml; it embeds a KSM config and API key - Remove the setup:pubsub script; scripts/setup_pubsub.js does not exist
Add Docker image release pipeline
GitHub Actions reported that setup-qemu-action@v3, setup-buildx-action@v3 and build-push-action@v6 target Node 20 and are being forced onto Node 24. - docker/setup-qemu-action v3 -> v4 - docker/setup-buildx-action v3 -> v4 - docker/build-push-action v6 -> v7 build-push-action v7 drops the DOCKER_BUILD_NO_SUMMARY and DOCKER_BUILD_EXPORT_RETENTION_DAYS environment variables and legacy build summary export, none of which are used here. actions/checkout@v6 and docker/login-action@v4 were not flagged and are unchanged.
Bump Docker actions off the deprecated Node 20 runtime
The smoke test resolved the image as IMAGE@digest and then passed --platform to docker run, which fails with "cannot overwrite digest" (exit 125): a digest pins a single manifest, so --platform cannot select a different one from the index. Reference the version tag instead, and pull each platform explicitly before running it so the correct architecture is present locally. Verified against the published keeper/gchat-app:v1.0.0-rc.1: both linux/amd64 and linux/arm64 report the expected architecture and exit with the expected configuration error.
Reference the image by tag in the release smoke test
Added documentation link for Google Chat app integration.
| if (defaultChannel && trimmedChannelId === defaultChannel) { | ||
| this.logger.info( | ||
| { channelId: trimmedChannelId }, | ||
| 'Boundary: default approval channel; no scope (traditional search)', | ||
| ); | ||
| return { folderUids: null, recordUids: null }; | ||
| } |
There was a problem hiding this comment.
When multi-channel scoping is enabled, this makes the default approval channel completely unrestricted. If a requester/approver is associated with a team that has allowed_folder_uids or allowed_record_uids, but the request is handled in the default channel, they can still get an unrestricted search. Is this intentional for backward compatibility, or should the default channel also resolve the user's team scope?
There was a problem hiding this comment.
This is intentional
| if (!submitted.ok) { | ||
| this.logger.warn( | ||
| { error: submitted.error, userEmail }, | ||
| 'list-team command failed', | ||
| ); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Could we distinguish a Commander/team lookup failure from a successful lookup where the user has no matching teams? Returning [] for both cases means the caller cannot tell whether the user genuinely has no mapped team or whether list-team failed. resolveApprovalChannel() then treats both cases as "no mapped team" and falls back to the default channel. For multi-channel routing, should a lookup failure instead be propagated/handled separately so we don't silently change the routing decision?
| } catch (error) { | ||
| this.logger.warn({ err: error, userEmail }, 'Failed to get user teams from Commander'); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
Could we distinguish a Commander/team lookup failure from a successful lookup where the user has no matching teams? Returning [] for both cases means the caller cannot tell whether the user genuinely has no mapped team or whether list-team failed. resolveApprovalChannel() then treats both cases as "no mapped team" and falls back to the default channel. For multi-channel routing, should a lookup failure instead be propagated/handled separately so we don't silently change the routing decision?
| await Promise.all( | ||
| uidList.map(async (uid) => { | ||
| try { | ||
| const item = await fetch.call(keeperClient, uid); |
There was a problem hiding this comment.
Could we limit the concurrency here if a team has a large allowed_*_uids list? This currently starts one Commander request per UID at the same time via Promise.all(). A large configured catalog could therefore generate a burst of concurrent Commander requests. A small concurrency limit or batching approach may make this more predictable.
4 participants
No description provided.