Skip to content

Conversation

@borisschlosser
Copy link
Contributor

This PR adds a changelog entry for BYOK and improved crypto capabilities for self-hosted as these feature will be enabled soon.

Additionally a breaking change warning is added as admins need to double check their encryption service permissions.

The self-hosted encryption service instructions are updated accordingly as well.

@claude
Copy link
Contributor

claude bot commented Nov 19, 2025

Documentation Review

I've reviewed the changes in this PR. Here are my findings:

Issues Found

File: content/docs/administration/self-hosting/changelog.md

Line 29: Missing period after "Breaking Change:"
The text should read "Breaking Change: Ensure..." but the full sentence needs a period at the end after "api/#encryption-services)." for proper punctuation.

Line 29: Link uses relative path with fragment
The link [here](/docs/administration/self-hosting/components/api/#encryption-services) includes a fragment identifier (#encryption-services). Please verify this section heading exists in the target file. Based on the diff, I see headings for "AWS KMS" and "Azure Key Vault" but no "Encryption services" heading.

Suggestion: Update the link to point to the correct section:

Breaking Change: Ensure your encryption service permissions are up-to-date with the instructions given [here](/docs/administration/self-hosting/components/api/#aws-kms) and [here](/docs/administration/self-hosting/components/api/#azure-key-vault).

Or better yet, make the link text more descriptive:

Breaking Change: Ensure your [AWS KMS](/docs/administration/self-hosting/components/api/#aws-kms) and [Azure Key Vault](/docs/administration/self-hosting/components/api/#azure-key-vault) encryption service permissions are up-to-date.

File: content/docs/administration/self-hosting/components/api.md

Line 134: Heading case issue
The heading "Mandatory key actions" should be "Mandatory key actions" - it correctly uses sentence case, so this is fine.

Line 134-135: Passive voice
The sentence "The key's key policy in AWS KMS must define the following actions" uses passive construction. Consider rewording for clarity:

Define the following actions in the key's key policy in AWS KMS. Otherwise, the service will fail to start or will not be able to run crypto operations:

Line 154: Heading readability
The heading "Mandatory key operations / permissions" mixes a slash separator. Consider using "or" for better readability:

#### Mandatory key operations or permissions

Line 157: Passive voice
"The key in Azure KeyVault must support and allow the following operations" could be more direct:

Configure the key in Azure KeyVault to support and allow the following operations. Otherwise, the service will fail to start or will not be able to run crypto operations:

Lines 165-167: Missing variable documentation
The table removes PULUMI_AZURE_KV_KEY_VERSION from the documentation. This appears to be intentional, but please confirm:

  • Is this variable no longer needed?
  • Should there be a note about removing it from existing configurations?
  • The description "Note: All previous versions of the key must remain enabled" is removed - is this requirement no longer valid?

Positive Aspects

✓ Proper use of warning notes shortcode
✓ Consistent formatting in lists
✓ Clear, actionable instructions
✓ Good addition of mandatory permissions section

Recommendations

  1. Verify the link anchor in the changelog entry points to a valid section heading
  2. Consider adding migration guidance if PULUMI_AZURE_KV_KEY_VERSION is being removed
  3. Review passive voice constructions for better clarity
  4. Ensure all new content ends with a newline (I cannot verify this from the diff alone)

Mention @claude if you'd like me to review any updates or provide additional feedback on specific sections.

@pulumi-bot
Copy link
Collaborator

@borisschlosser
Copy link
Contributor Author

Lines 165-167: Missing variable documentation The table removes PULUMI_AZURE_KV_KEY_VERSION from the documentation. This appears to be intentional, but please confirm:

  • Is this variable no longer needed?
  • Should there be a note about removing it from existing configurations?
  • The description "Note: All previous versions of the key must remain enabled" is removed - is this requirement no longer valid?

This is intentional. The variable is not needed anymore (for a longer time). This is a non-breaking change as the variable is just ignored if still set. Azure KeyVault now always uses the latest version to encrypt data and will use the key version stored besides the data to decrypt it. The info that older keys must not be deleted or disabled can be found in the info box above the table.

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@pulumi-bot
Copy link
Collaborator

@meagancojocar meagancojocar merged commit 31bb9f5 into master Nov 20, 2025
15 checks passed
@meagancojocar meagancojocar deleted the boris/byok-self-hosted branch November 20, 2025 19:15
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.

6 participants