Skip to content

checking if useoidc env var is set #335

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 6 commits into from
Jun 14, 2024

Conversation

polatengin
Copy link
Contributor

@polatengin polatengin commented Jun 12, 2024

To be able to remove hard-coded use_oidc = {true, false} config from provider section, provider should be able to determine if a specific environment variable is set.

Before;

provider "powerplatform" {
  use_oidc = true
}

After (with POWER_PLATFORM_USE_OIDC environment variable is set);

provider "powerplatform" {
}

closes #329

@polatengin polatengin self-assigned this Jun 12, 2024
@polatengin polatengin linked an issue Jun 12, 2024 that may be closed by this pull request
eduardodfmex
eduardodfmex previously approved these changes Jun 12, 2024
Copy link
Contributor

@eduardodfmex eduardodfmex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this change that will help to carry the OIDC.

ianjensenisme
ianjensenisme previously approved these changes Jun 12, 2024
Copy link
Contributor

@ianjensenisme ianjensenisme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the environment config trump the provider config? This is more of a "food for thought" question, as I've also added variable handling that uses this priority order, so not worth changing, but I wonder if we've gotten it backwards throughout the provider. I'm probably just not thinking of an obvious reason to do things in that priority order. Anywho, LGTM.

@polatengin
Copy link
Contributor Author

Why does the environment config trump the provider config? This is more of a "food for thought" question, as I've also added variable handling that uses this priority order, so not worth changing, but I wonder if we've gotten it backwards throughout the provider. I'm probably just not thinking of an obvious reason to do things in that priority order. Anywho, LGTM.

it's more like, the options that you can use to configure the plan/apply in the way you like.

so, instead of having how you authenticate hard-coded, you might want to configure it in the pipeline, based on the pipeline parameters, etc.

quick note; this change doesn't give priority to environment config, we still check if the config exists in the provider section, if yes, we use it, if no, we check the environment variables.

@polatengin polatengin dismissed stale reviews from ianjensenisme and eduardodfmex via 8058056 June 12, 2024 17:57
@polatengin polatengin requested a review from mattdot June 12, 2024 17:58
@polatengin polatengin enabled auto-merge (squash) June 12, 2024 18:51
@ianjensenisme
Copy link
Contributor

Why does the environment config trump the provider config? This is more of a "food for thought" question, as I've also added variable handling that uses this priority order, so not worth changing, but I wonder if we've gotten it backwards throughout the provider. I'm probably just not thinking of an obvious reason to do things in that priority order. Anywho, LGTM.

it's more like, the options that you can use to configure the plan/apply in the way you like.

so, instead of having how you authenticate hard-coded, you might want to configure it in the pipeline, based on the pipeline parameters, etc.

quick note; this change doesn't give priority to environment config, we still check if the config exists in the provider section, if yes, we use it, if no, we check the environment variables.

Ah, yep, I stared at it too long and lost the plot: we check if the config value is blank and only then would we use the environment variable, that makes more sense to me. I do basically the same thing with envOidcRequestUrl and envOidcRequestToken . Thanks for following up.

mawasile
mawasile previously approved these changes Jun 13, 2024
eduardodfmex
eduardodfmex previously approved these changes Jun 13, 2024
mattdot
mattdot previously approved these changes Jun 13, 2024
@polatengin polatengin dismissed stale reviews from mattdot, eduardodfmex, and mawasile via 9d014b3 June 14, 2024 14:00
@eduardodfmex
Copy link
Contributor

LGTM.

Copy link
Contributor

@eduardodfmex eduardodfmex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@polatengin polatengin merged commit ecb6c60 into main Jun 14, 2024
@polatengin polatengin deleted the enpolat/329-use-oidc-as-env-variable branch June 14, 2024 19:04
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.

Use OIDC as Env Variable
5 participants