Skip to content

feat: add for customize attribute mapping and change default google subject map #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 29, 2025

Conversation

Monska85
Copy link
Contributor

@Monska85 Monska85 commented May 29, 2025

PR Type

Enhancement


Description

• Add customizable attribute mapping variable for GCP Workload Identity
• Change default google.subject mapping to prevent long branch issues
• Include validation for required google.subject mapping
• Update documentation with new variable details


Changes walkthrough 📝

Relevant files
Enhancement
main.tf
Replace hardcoded mapping with variable                                   

main.tf

• Replace hardcoded attribute_mapping with variable reference
• Remove
inline attribute mapping configuration

+2/-12   
variables.tf
Add attribute mapping variable with validation                     

variables.tf

• Add gcp_workload_identity_pool_provider_attribute_mapping variable

Set new default google.subject mapping using user_email, project_id,
job_id
• Include validation for required google.subject field
• Add
assertion_sub attribute mapping

+20/-0   
Documentation
CHANGELOG.md
Document attribute mapping changes                                             

CHANGELOG.md

• Document new attribute mapping variable addition
• Explain change
from assertion.sub to composite google.subject
• Note prevention of
long branch name issues

+12/-0   
README.md
Update variable documentation                                                       

README.md

• Add documentation for new attribute mapping variable
• Show default
mapping values in table format

+1/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @sparkfabrik-ai-bot
    Copy link

    sparkfabrik-ai-bot bot commented May 29, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 8eabcec)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Validation Logic

    The validation condition uses complex boolean logic that could be simplified and made more readable. Consider breaking down the condition or using more descriptive intermediate variables.

    condition     = length(var.gcp_workload_identity_pool_provider_attribute_mapping) > 0 && var.gcp_workload_identity_pool_provider_attribute_mapping["google.subject"] != null && length(var.gcp_workload_identity_pool_provider_attribute_mapping["google.subject"]) > 0
    Breaking Change

    The default value for google.subject has changed significantly from a simple assertion.sub to a complex concatenated string. This is a breaking change that may affect existing deployments and should be carefully validated.

    "google.subject"                 = "assertion.user_email+\"::\"+assertion.project_id+\"::\"+assertion.job_id"

    @Monska85 Monska85 force-pushed the feat/change_attribute_mapping branch 4 times, most recently from 594f2be to 35aebcc Compare May 29, 2025 16:06
    @Monska85
    Copy link
    Contributor Author

    /improve

    @Monska85
    Copy link
    Contributor Author

    /review

    @sparkfabrik-ai-bot
    Copy link

    Persistent review updated to latest commit 35aebcc

    @Monska85 Monska85 force-pushed the feat/change_attribute_mapping branch from 35aebcc to 8eabcec Compare May 29, 2025 16:11
    @Monska85
    Copy link
    Contributor Author

    /improve

    @Monska85
    Copy link
    Contributor Author

    /review

    @sparkfabrik-ai-bot
    Copy link

    sparkfabrik-ai-bot bot commented May 29, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 8eabcec

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use safe key existence check

    The validation condition should use contains() to safely check if the key exists
    before accessing it. Direct key access can cause errors if the key doesn't exist in
    the map.

    variables.tf [39]

    -condition     = length(var.gcp_workload_identity_pool_provider_attribute_mapping) > 0 && var.gcp_workload_identity_pool_provider_attribute_mapping["google.subject"] != null && length(var.gcp_workload_identity_pool_provider_attribute_mapping["google.subject"]) > 0
    +condition     = length(var.gcp_workload_identity_pool_provider_attribute_mapping) > 0 && contains(keys(var.gcp_workload_identity_pool_provider_attribute_mapping), "google.subject") && length(var.gcp_workload_identity_pool_provider_attribute_mapping["google.subject"]) > 0
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential runtime error when accessing var.gcp_workload_identity_pool_provider_attribute_mapping["google.subject"] without first checking if the key exists. Using contains() prevents Terraform from failing if the key is missing from the map.

    Medium

    Previous suggestions

    Suggestions up to commit 35aebcc
    CategorySuggestion                                                                                                                                    Impact
    General
    Fix missing concatenation operator

    There's a missing + operator before assertion.job_id in the changelog description.
    This creates confusion about the actual implementation.

    CHANGELOG.md [25]

    -- Change the default attribute mapping for the GCP Workload Identity Pool Provider from `google.subject = assertion.sub` to `google.subject = assertion.user_email+"::"+assertion.project_id+"::"assertion.job_id`.
    +- Change the default attribute mapping for the GCP Workload Identity Pool Provider from `google.subject = assertion.sub` to `google.subject = assertion.user_email+"::"+assertion.project_id+"::"+assertion.job_id`.
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly identifies a missing + operator in the changelog description, which improves documentation accuracy and clarity for users reading about the breaking change.

    Low
    Suggestions up to commit f0cd3ba
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix string concatenation syntax error

    The string concatenation is missing a '+' operator before the last part. This will
    cause a syntax error in the attribute mapping expression.

    variables.tf [28]

    -"google.subject"          = "assertion.user_email+\"::\"+assertion.project_id+\"::\"assertion.job_id"
    +"google.subject"          = "assertion.user_email+\"::\"+assertion.project_id+\"::\"+assertion.job_id"
    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical syntax error in the string concatenation where the + operator is missing before assertion.job_id. This would cause the Terraform configuration to fail during validation or apply.

    High

    @sparkfabrik-ai-bot
    Copy link

    Persistent review updated to latest commit 8eabcec

    @Monska85 Monska85 force-pushed the feat/change_attribute_mapping branch from 8eabcec to a353000 Compare May 29, 2025 16:14
    @Monska85
    Copy link
    Contributor Author

    /improve

    @sparkfabrik-ai-bot
    Copy link

    sparkfabrik-ai-bot bot commented May 29, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @Monska85 Monska85 merged commit c2acb40 into main May 29, 2025
    1 check passed
    @Monska85 Monska85 deleted the feat/change_attribute_mapping branch May 29, 2025 16:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant