Skip to content

Sync nf-core joiners to Seqera workspace - #24

Open
adamrtalbot wants to merge 4 commits into
mainfrom
feat/seqera-megatests-sync
Open

adamrtalbot wants to merge 4 commits into
mainfrom
feat/seqera-megatests-sync

Conversation

@adamrtalbot

@adamrtalbot adamrtalbot commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Why

New nf-core joiners should get read-only access to the megatests workspace without a separate manual step, and should lose it when they leave the nf-core GitHub org.

What

  • Add Slack joiners to the nf-core/megatests Seqera workspace with the nf-core-read-only role, for new GitHub invitees and for people already in the org.
  • Clear outcomes: the add returns a reason (added, already a participant, no email, no Seqera account, API error with its HTTP status). Nothing is said when the SEQERA_* params aren't set.
  • Accurate messages: the Seqera add runs before the welcome, which now mentions megatests access. Existing members no longer get "nothing to do" when the bot then adds them. The caller's DM explains any failure.
  • Optional Platform email: the shortcut reads an optional "Seqera Platform email" answer from the GitHub Invitation Request workflow form and falls back to the Slack email. The form question itself is added in Slack Workflow Builder.
  • Records: each add is stored in DynamoDB (PK=SEQERA#<workspace-id> SK=USER#<slack-user-id>: GitHub username, email used, participant ID).
  • Weekly removal sync (scheduler/seqera_jobs.py, Mondays 09:00 UTC): removes recorded users who have neither an active membership nor a pending invite in the nf-core GitHub org. It only touches people the bot added, and only that workspace.
  • Dry run: SEQERA_REMOVAL_DRY_RUN (default true) logs who would be removed without removing anyone.
  • Tests for all of the above. The setup, form question, removal job and dry run are documented in docs/deployment.md.

The slash command is tracked in #25.

🤖 Generated with Claude Code

https://claude.ai/code/session_01A4T79rC8VmKnzeJDnnFnge

@adamrtalbot

Copy link
Copy Markdown
Collaborator Author

@FriederikeHanssen merge when stuff is set up

@ewels

ewels commented Oct 17, 2026

Copy link
Copy Markdown
Member

This looks good, thanks! One addition I'd like is for it to remove users on sync as well. For example if we remove someone from the GitHub org id like them to also get removed from platform.

Also is this going to spam people? I guess everyone who joins nf-core will get an invite email to platform?

ewels commented Oct 17, 2026

Copy link
Copy Markdown
Member

🤖 This reply was written by a Claude Code session on @ewels's behalf, not by Phil directly.

1. Removing users on sync

There's no real "sync" in this PR yet. It's event-driven: a user is added only when github add runs for them, so nothing notices when someone leaves the org. Removal would need a periodic job. Suggestion:

  • When the bot adds someone, store a record in DynamoDB (e.g. PK=SEQERA#<slack-user-id>, with the email, GitHub username and Seqera user ID).
  • Add a weekly job to scheduler/ (same pattern as the on-call Monday jobs). It diffs those records against GitHub org members and removes anyone who has left the org from the workspace.
  • Only remove people the bot added itself, so participants added by hand stay put. A log-only dry-run for the first week or two would be a sensible safety net.

Matching on records the bot wrote avoids the hard part, which is mapping GitHub users to Seqera emails after the fact.

2. Spam

It's limited to one call per person, at the moment they're invited (or when someone runs github add on an existing member). Nobody gets a bulk backfill. Two things are still worth checking before merging:

  • What does participants/add do for an email with no Seqera account, or one that isn't yet in the org? If it creates an org invite and sends an email, every new joiner gets an unprompted Platform email. That may be fine, but it's worth confirming with a throwaway address. The docs say users "must already have a Seqera account", so if that's the intent, the bot could look the user up first and skip silently when there's no account.
  • The Slack email often won't match the email people use to log in to Seqera (e.g. a work address vs. a personal one). Those cases will either fail or invite the wrong address. It might be worth mentioning the megatests access in the welcome message so people know to ask if it didn't work.

Generated by Claude Code

ewels commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

🤖 This reply was written by a Claude Code session on @ewels's behalf, not by Phil directly.

Suggestion: a dedicated /nf-core seqera command

The auto-sync covers new joiners. A separate command would let admins manage Seqera access directly, for corrections, for people whose Slack email differs from their Seqera email, and for groups other than megatests:

/nf-core seqera add @user <group> [--email foo@bar.com]
/nf-core seqera remove @user <group>
/nf-core seqera list [@user]          # groups a user is in, or all groups
  • Groups defined in code. Keep a typed mapping of friendly names to Seqera targets in the repo, e.g. a SEQERA_GROUPS dict of dataclasses in checks/seqera.py. For example, "megatests": SeqeraGroup(workspace_id=..., role="nf-core-read-only") and "maintainers": SeqeraGroup(team_id=...). Changes then go through PR review and mypy, with no extra SSM params per group. Only the token needs to stay in SSM.
  • One shared module. Put the add/remove/list calls in checks/seqera.py. The joiner auto-sync becomes a call to add(user, "megatests"), and the removal job from my earlier comment reuses remove.
  • Permissions. Restrict it to @core-team, checked centrally in the router the way on-call does it.
  • Email override. --email fixes the Slack/Seqera email mismatch without anyone touching the Seqera UI.
  • Track it. Record each add in DynamoDB so list and the removal job know what the bot manages.
  • Follow the usual Slack rules: ack() straight away, ephemeral responses, and add the command to the help text.

Slack messages don't yet reflect the Seqera step

The PR hasn't changed since the first review (still the same 2 commits), so these haven't been edited yet:

  1. Existing members. The channel reply says "already a member of the nf-core GitHub org — nothing to do.", then the bot goes on to add them to Seqera. The public message should say what actually happened, e.g. "already in the GitHub org; added to the Seqera megatests workspace".
  2. New joiners. The thread welcome is posted before the Seqera sync runs and never mentions it. Run the sync first and add a line like "You've also been given read-only access to the nf-core megatests workspace on Seqera Platform". That also covers the earlier concern about people being surprised by a Platform email.
  3. The failure message is too vague. add_to_megatests returns False both when sync isn't configured and when it actually fails. So with no SEQERA_* params set, every invite DMs "Seqera workspace sync did not complete…". Suggest returning a reason, e.g. a CheckResult(ok, message) like checks/github.py uses. The DM can then say why: not configured (say nothing), no Slack email, no matching Seqera account, or an HTTP error with its status.

Generated by Claude Code

@adamrtalbot

Copy link
Copy Markdown
Collaborator Author

So we tried doing a sync via IAC (Alchemy) on nf-core/ops. It looked practical but most users had a different email between GitHub and Seqera Platform, resulting in us requiring a mapping file between most users which needs to be manually maintained, so the sync breaks down.

This felt like the smallest MVP: add users with the view role to a workspace when they ask to join nf-core. This means every user is added to the workspace automatically, they can then be migrated to the maintainer or core teams by some downstream process. Spam wise, it's just every user receiving one extra email so I don't think it's a big deal.

Adding a separate slash command would work, but doesn't really solve the core syncing problem.

@adamrtalbot

Copy link
Copy Markdown
Collaborator Author

Let's incorporate the fixes, and make the process an add and sync for the users to the workspace.

Follow up issue for the slash command.

Do we already have a dynamoDB we can store info in?

@adamrtalbot

Copy link
Copy Markdown
Collaborator Author

Summary of the changes for this PR:

  1. Clearer outcomes. The Seqera add should report why it failed, not just pass/fail. If sync isn't configured, say nothing. Otherwise distinguish no email, no matching account, and API error.
  2. Accurate messages for existing members. The public reply shouldn't say "nothing to do" when the bot then adds them to the workspace.
  3. Mention megatests access in the welcome. For new joiners, run the Seqera add before the welcome message and say they now have read-only access to the megatests workspace.
  4. Optional Platform email. Add a question to the "GitHub Invitation Request" workflow form for a separate Platform email. Use it if given, otherwise fall back to the Slack email. The form change is made in Slack Workflow Builder, outside this repo.
  5. Record each add. Store who the bot added in the existing DynamoDB table: Slack user, GitHub username, email used and workspace.
  6. Weekly removal sync. A scheduled job checks each recorded user against the nf-core GitHub org and removes anyone who has left from the megatests workspace. It only touches people the bot added, and only that workspace.
  7. Dry-run first. A setting so the removal job only logs for the first week or two.
  8. Tests and docs. Cover the new behaviour and document the removal job, dry-run setting and new form question.

The slash command is out of scope and tracked in #25.

- Return a SeqeraResult with a reason (not configured, no email, no
  account, API error) instead of a bool; stay silent when unconfigured.
- Add to the workspace before the welcome and mention megatests access;
  existing-member replies no longer say "nothing to do" after an add.
- Use an optional Platform email from the GitHub Invitation Request form,
  falling back to the Slack email.
- Record each add in DynamoDB (PK=SEQERA#<workspace> SK=USER#<slack-id>).
- Weekly job removes recorded users with no active or pending GitHub org
  membership; log-only while SEQERA_REMOVAL_DRY_RUN is on (default).
- Tests and docs for the above.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4T79rC8VmKnzeJDnnFnge

Copy link
Copy Markdown
Collaborator Author

Pushed 24cc242, which covers all 8 items from the summary above. CI is green.

Before merging:

  • The Seqera spec doesn't say what participants/add returns for an email with no account. The code treats 404 as "no account", and anything else shows up in the DM as an API error with its status. Worth one test with a throwaway address.
  • Add the optional "Seqera Platform email" question to the GitHub Invitation Request form in Workflow Builder.
  • The removal job starts in dry-run mode. Check the [dry run] Would remove logs, then set SEQERA_REMOVAL_DRY_RUN=false.

Generated by Claude Code

@adamrtalbot adamrtalbot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good overall. Two things before merging:

1. The Seqera SSM secrets are required, not optional (infra/stacks/bot_stack.py)
SEQERA_TOKEN, SEQERA_ORG_ID and SEQERA_WORKSPACE_ID are added to the ECS secrets list. If the stack is deployed before all three parameters exist in SSM, the task fails to start and the whole bot goes down, not just the Seqera step. docs/deployment.md says a missing parameter means "the step is skipped silently", which isn't true for the deployed bot. Please either fix the docs to say the parameters must exist before deploying, or make these secrets optional.

2. Check how the add response codes are mapped (checks/seqera.py)

  • A 404 is treated as "no Seqera account", but the OpenAPI spec for participants/add doesn't list a 404. It lists only 200, 400, 403 and 409. An email with no account may come back as something else, such as a 400. Since most new joiners won't have a Seqera account, this path will be the common one. Could you test one add with an email that has no account and adjust the mapping to match?
  • The spec says a 409 means "Duplicate element or collaborator not permitted for organization with active SSO", but every 409 is reported as "already a participant". This only matters if the nf-core org uses SSO, so it's worth a quick check.

Keeping removal dry-run on for the first week or two, as the PR already suggests, sounds right.


Generated by Claude Code

- A 409 from participants/add also means "collaborator not permitted for
  organization with active SSO", so only report "already a participant"
  after a participant search finds the email; otherwise report the 409
  and Seqera's message.
- Include Seqera's own message in the "no account" (404) result, since
  that mapping isn't in the spec and is unconfirmed.
- Docs: the SEQERA_* SSM parameters are ECS secrets and must exist before
  deploying, or the task fails to start.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A4T79rC8VmKnzeJDnnFnge

Copy link
Copy Markdown
Collaborator Author

Addressed in e716614:

  1. SSM secrets: fixed the docs. docs/deployment.md now says all three SEQERA_* parameters must exist before deploying, because a missing one stops the ECS task from starting. "Skipped silently" now only describes local runs. I left the stack as is rather than adding conditional secret logic.
  2. Response codes:
    • 409: reported as "already a participant" only when GET .../participants?search=<email> finds that email. Otherwise it's reported as an HTTP 409 error with Seqera's message, which covers the SSO case.
    • No account: still mapped from 404, and the DM now includes Seqera's own message too. I haven't tested an add with an account-less email because this session has no Seqera token. If it comes back as a 400 or anything else, the DM will show that status and message, and I'll adjust the mapping to match.

Generated by Claude Code

@adamrtalbot

Copy link
Copy Markdown
Collaborator Author

@ewels what do you think of this now?

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

Successfully merging this pull request may close these issues.

4 participants