Skip to content
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

fixed up nit from PR 1666 #1676

Merged
merged 1 commit into from
Sep 14, 2023
Merged

fixed up nit from PR 1666 #1676

merged 1 commit into from
Sep 14, 2023

Conversation

dgulli
Copy link
Collaborator

@dgulli dgulli commented Sep 14, 2023

Fixed the nit from PR 1666 as per recommendation

#1666 (comment)


Checklist

I applicable, I acknowledge that I have:

  • Read the contributing guide
  • Ran terraform fmt on all modified files
  • Regenerated the relevant README.md files using tools/tfdoc.py
  • Made sure all relevant tests pass

@olliefr
Copy link
Collaborator

olliefr commented Sep 14, 2023

@dgulli what do you think about the following approach?

  purpose = try(
    each.value.purpose,
    each.value.global ? "GLOBAL_MANAGED_PROXY" : "REGIONAL_MANAGED_PROXY"
  )

My point is that you can use a Boolean variable/value directly in the conditional expression 🙂

@dgulli dgulli enabled auto-merge September 14, 2023 04:56
@dgulli
Copy link
Collaborator Author

dgulli commented Sep 14, 2023

@dgulli what do you think about the following approach?

  purpose = try(
    each.value.purpose,
    each.value.global ? "GLOBAL_MANAGED_PROXY" : "REGIONAL_MANAGED_PROXY"
  )

My point is that you can use a Boolean variable/value directly in the conditional expression 🙂

Hey,

Yes this was brought up in my original PR too, but from a readability perspective its not initially obvious to the casual reader, so i omitted it :)

@dgulli dgulli requested a review from ludoo September 14, 2023 04:57
@dgulli dgulli merged commit eab298f into master Sep 14, 2023
13 checks passed
@dgulli dgulli deleted the remediate_nit_from_PR1666 branch September 14, 2023 05:23
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.

3 participants