[firestartr-bootstrap] Several fixes for new errors found#403
Conversation
frmadem
changed the title
firestartr-bootstrap] Ensure org is in lowercase for AWS
frmadem
changed the title
firestartr-bootstrap] Ensure org is in lowercase for AWSfirestartr-bootstrap] Several fixes for new errors found
There was a problem hiding this comment.
Pull request overview
This pull request addresses multiple issues discovered during the Firestartr bootstrap process for new customers. The main focus is on fixing hardcoded values, ensuring proper credential separation between admin and operator operations, enforcing lowercase organization names for AWS Parameter Store references, and correcting the default group assignment for domain and system resources.
Changes:
- Added
DefaultGroupfield to allow proper ownership assignment for default domains and systems (fixes issue #402) - Implemented
GhOrgLowerCasefield to enforce lowercase organization names in AWS Parameter Store paths (addresses issue #400) - Separated GitHub App credentials into admin (
GithubApp) and operator (GithubAppOperator) credentials with dedicated secret stores (fixes issue #399) - Removed unnecessary
BotNamefield and references from the codebase - Fixed ArgoCD application path formatting to remove duplicate parameters
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| firestartr-bootstrap/types.go | Added DefaultGroup field to Bootstrap struct; added GithubAppOperator field to CredsFile; removed BotName field; updated GitHub App YAML tag from githubApp to github |
| firestartr-bootstrap/templates/initial_claims.tmpl | Updated domain and system owners from {{ $.Org }}-all to {{ $.DefaultGroup }} |
| firestartr-bootstrap/schemas/credentials-file.json | Updated schema to use github instead of githubApp; removed owner and botName requirements; kept only prefappBotPat and operatorPat |
| firestartr-bootstrap/schemas/bootstrap-file.json | Added defaultGroup property and marked it as required |
| firestartr-bootstrap/main.go | Added GhOrgLowerCase field; implemented lowercase conversion for org names; updated credential owner assignment; used lowercased org in calculateParameters |
| firestartr-bootstrap/kubernetes.go | Added operator secrets file constant and credential mapping; implemented separate credential population for operator GitHub App; applied PEM escaping for both credential sets |
| firestartr-bootstrap/helm.go | Updated operator Helm values to use GithubAppOperator credentials instead of admin credentials |
| firestartr-bootstrap/external_secrets/operator_secrets.tmpl | New template for operator-specific secrets with AWS Parameter Store integration |
| firestartr-bootstrap/commands.go | Removed BotName assignment statements |
| firestartr-bootstrap/argocd.go | Fixed ArgoCD application path formatting by removing duplicate parameters and updating base path |
| firestartr-bootstrap/README.md | Updated documentation for defaultGroup parameter; removed documentation for obsolete githubApp.owner and githubApp.botName fields; updated schema references from githubApp to github |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tstrap-of-a-new-customer' of https://github.com/prefapp/daggerverse into fix/399-firestartr-bootstrap-errors-detected-in-the-bootstrap-of-a-new-customer
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…-in-the-bootstrap-of-a-new-customer
…-in-the-bootstrap-of-a-new-customer
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| WithExec([]string{ | ||
| "gh", "api", fmt.Sprintf("/orgs/%s/teams/%s", m.GhOrg, m.Bootstrap.DefaultGroup), | ||
| }). |
There was a problem hiding this comment.
This GitHub API endpoint expects a team slug (and should be URL path-escaped). Passing DefaultGroup verbatim will fail for teams whose display name contains spaces or other characters (the README example uses a space). Either validate/require defaultGroup to be the team slug (e.g., my-group) and document it, or apply url.PathEscape (and/or resolve the slug by name via the API) before calling gh api.
| patchedDir, err := safelyPatchYamlConfig( | ||
| ctx, | ||
| argoCDRepo.Directory("/repo"), | ||
| "kubernetes-sys-services/firestartr-pro/argo-configuration-secrets/values.yaml", | ||
| fmt.Sprintf("kubernetes-sys-services/firestartr-%s/argo-configuration-secrets/values.yaml", m.Bootstrap.Env), | ||
| m.Bootstrap.Org, | ||
| clientAccess, |
There was a problem hiding this comment.
installationIdSecretRef still uses m.Bootstrap.Org in the Parameter Store path, but the rest of the PR introduces GhOrgLowerCase to ensure org references are lowercase for SSM keys. This will reintroduce the original casing bug for the ArgoCD installation-id lookup. Use m.GhOrgLowerCase when building this parameter path.
firestartr-bootstrap/README.md
Outdated
| defaultOrgPermissions: view | ||
| defaultBranchStrategy: none | ||
| defaultFirestartrGroup: firestartr | ||
| defaultGroup: 'my group' # must be an existing group/team or one of the created by the bootstrap process |
There was a problem hiding this comment.
The example defaultGroup: 'my group' is likely not a valid Firestartr GroupClaim / GitHub team slug (spaces will also fail the new gh api /orgs/<org>/teams/<defaultGroup> check). Consider documenting that defaultGroup must be a GitHub team slug (e.g., my-group), and clarify whether this group is expected to already exist (it is not created in templates/initial_claims.tmpl).
| defaultGroup: 'my group' # must be an existing group/team or one of the created by the bootstrap process | |
| defaultGroup: my-group # GitHub team slug for the default Firestartr group; must refer to an existing team in the org (it is not created by the bootstrap) |
juanjosevazquezgil
deleted the
fix/399-firestartr-bootstrap-errors-detected-in-the-bootstrap-of-a-new-customer
branch
Solves
firestartr-bootstrap] Ensure ghOrg is in lower case whenever a parameter store element is referenced #400firestartr-bootstrap] Errors detected in the bootstrap of a new customer #399 (comment)firestartr-bootstrap] Errors detected in the bootstrap of a new customer #399firestartr-bootstrap] Errors detected in the bootstrap of a new customer #399 (comment)firestartr-bootstrap]<org>-allgroup is added as owner of the default domain & system #402firestartr-bootstrap] Errors detected in the bootstrap of a new customer #399 (comment)