Enable user management, workspace group creation, and auto-consent by default in config sample - #5068
Enable user management, workspace group creation, and auto-consent by default in config sample#5068Marcus Robinson (marrobi) with Copilot wants to merge 8 commits into
Conversation
…ps schema default Co-authored-by: marrobi <17089773+marrobi@users.noreply.github.com>
|
Copilot address comemnts. |
Co-authored-by: marrobi <17089773+marrobi@users.noreply.github.com>
Unit Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 6ca0d3e. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
🟡 Changes recommended
The authentication documentation names a different Entra permission from the one deployment actually grants.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables user-management features by default and prevents workspace edits from disabling Entra role groups.
Changes:
- Enables three authentication and user-management settings.
- Aligns workspace schema and Porter defaults.
- Adds regression coverage, documentation, versioning, and changelog updates.
File summaries
| File | Description |
|---|---|
config.sample.yaml |
Enables the three features by default. |
templates/workspaces/base/template_schema.json |
Defaults AAD group creation to true. |
templates/workspaces/base/porter.yaml |
Bumps the workspace bundle version. |
docs/tre-admins/environment-variables.md |
Documents sample defaults and implications. |
docs/tre-admins/auth.md |
Updates authentication defaults documentation. |
api_app/tests_ma/test_templates/test_workspace_base_template_defaults.py |
Verifies schema and Porter defaults remain aligned. |
api_app/tests_ma/test_templates/__init__.py |
Marks the test package. |
CHANGELOG.md |
Records the enhancement and regression fix. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The documentation references a nonexistent Terraform default, and the new test relies on an undeclared direct dependency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
api_app/tests_ma/test_templates/test_workspace_base_template_defaults.py:5
- This test directly imports PyYAML, but neither
api_app/requirements.txtnorapi_app/requirements-dev.txtdeclares it. The test image currently depends on PyYAML arriving transitively through another package, so an unrelated dependency change can break test collection. Add a pinned PyYAML dependency torequirements-dev.txt.
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The regression test cannot access template files in the API test image and is not triggered by template-only changes.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| REPO_ROOT = Path(__file__).resolve().parents[3] | ||
| BASE_WORKSPACE_DIR = REPO_ROOT / "templates" / "workspaces" / "base" |
There was a problem hiding this comment.
Moved the regression check to tests/test_workspace_base_template_defaults.py so it runs from the repository root, outside the API image context. The CI step now executes it from the root in commit 70953788.
| raise AssertionError(f"Property '{PARAM_NAME}' not found in {schema_path}") | ||
|
|
||
|
|
||
| def test_workspace_base_create_aad_groups_defaults_are_consistent_and_true(): |
There was a problem hiding this comment.
Added a repository-level CI filter for templates/workspaces/base/porter.yaml and template_schema.json, with a dedicated validation step running the regression check. This is in commit 70953788.
Co-authored-by: marrobi <17089773+marrobi@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The empty API file triggers the stale-version CI failure, and the new test is omitted from its own workflow filter.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/9 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
||
| - name: Workspace base template defaults | ||
| if: ${{ steps.filter.outputs.workspace_base_template == 'true' }} | ||
| run: python tests/test_workspace_base_template_defaults.py |
There was a problem hiding this comment.
Removed the empty api_app/tests_ma/test_templates/__init__.py so this non-API change no longer trips the stale API version gate. Commit 0a624a33.
Co-authored-by: marrobi <17089773+marrobi@users.noreply.github.com>
What is being addressed
config.sample.yamlshipped withuser_management_enabled,auto_workspace_group_creation, andauto_grant_workspace_consentall set tofalse, so users following the default deployment path miss out on UI-based user/workspace assignment, automatic AAD security group creation, and auto-granted consent — requiring extra manual configuration for a good out-of-the-box experience.While validating this, a related regression was found:
templates/workspaces/base/template_schema.jsondefaulted the updateablecreate_aad_groupsproperty tofalsewhileporter.yamldefaulted it totrue. Since RJSF materializes the schema default whenever an older workspace has no stored value for the property, submitting an unrelated edit to an existing workspace could silently sendfalse, causing Terraform to destroy the workspace's AAD role groups and app-role assignments.How is this addressed
user_management_enabled,auto_workspace_group_creation, andauto_grant_workspace_consenttotrueinconfig.sample.yaml, with inline comments noting the Entra ID licensing implications of group creation.docs/tre-admins/environment-variables.mdanddocs/tre-admins/auth.mdto reflect the new sample defaults and reiterate the additional Entra permissions/licensing trade-offs.templates/workspaces/base/template_schema.json'screate_aad_groupsdefault totrue, matchingporter.yaml.api_app/tests_ma/test_templates/test_workspace_base_template_defaults.py, which parses bothporter.yamlandtemplate_schema.jsonand fails if thecreate_aad_groupsdefaults diverge or are nottrue.tre-workspace-basefrom2.10.1to2.10.2and updatedCHANGELOG.mdunder Unreleased (ENHANCEMENTS and BUG FIXES).