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

support passing impersonate-service-account #463

Conversation

ericyz
Copy link
Collaborator

@ericyz ericyz commented Mar 23, 2020

No description provided.

Copy link
Contributor

@morgante morgante left a comment

Choose a reason for hiding this comment

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

@@ -25,12 +25,12 @@ CLUSTER_NAME=$2

echo "Waiting for cluster $CLUSTER_NAME in project $PROJECT to reconcile..."

current_status=$(gcloud container clusters list --project="$PROJECT" --filter=name:"$CLUSTER_NAME" --format="value(status)")
current_status=$(gcloud container clusters list --project="$PROJECT" --filter=name:"$CLUSTER_NAME" --format="value(status)") --impersonate_service_account="$impersonate_service_account"
Copy link
Member

Choose a reason for hiding this comment

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

I believe you will need to add an argument for the $impersonate_service_account
similar to here:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Bharath!! I missed out this. Will fix it

@@ -25,12 +25,12 @@ CLUSTER_NAME=$2

echo "Waiting for cluster $CLUSTER_NAME in project $PROJECT to reconcile..."

current_status=$(gcloud container clusters list --project="$PROJECT" --filter=name:"$CLUSTER_NAME" --format="value(status)")
current_status=$(gcloud container clusters list --project="$PROJECT" --filter=name:"$CLUSTER_NAME" --format="value(status)") --impersonate_service_account="$impersonate_service_account"
Copy link
Member

Choose a reason for hiding this comment

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

Also wouldn't we need to handle the case that impersonate_service_account is empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this would work as expected with empty impersonate_service_account - no impersonation for the command.

This can be tested with any gcloud command, such as gcloud projects list --impersonate_service_account=""

variable "impersonate_service_account" {
description = "An optional service account to impersonate. This cannot be used with credentials_path. If this service account is not specified and credentials_path is absent, the module will use Application Default Credentials."
default = ""
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: newline

variable "impersonate_service_account" {
description = "An optional service account to impersonate. This cannot be used with credentials_path. If this service account is not specified and credentials_path is absent, the module will use Application Default Credentials."
default = ""
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: newline

@ericyz
Copy link
Collaborator Author

ericyz commented Mar 26, 2020

Updated

@ericyz ericyz requested a review from morgante March 26, 2020 01:47
@ericyz
Copy link
Collaborator Author

ericyz commented Mar 26, 2020

@morgante - Do you mind explaining what auto-gen does and how auto-gen works?

@morgante
Copy link
Contributor

@morgante - Do you mind explaining what auto-gen does and how auto-gen works?

It's explained in the contributing guide. Please read: https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/CONTRIBUTING.md

@ericyz ericyz force-pushed the feature/impersonate-service-account branch from 3e3c44c to 52f0918 Compare April 14, 2020 10:48
@ericyz ericyz requested a review from a team as a code owner April 14, 2020 10:48
@ericyz
Copy link
Collaborator Author

ericyz commented Apr 14, 2020

Hey, I have updated accordingly. Would be another chance to take a look?

@bittermandel
Copy link

Would love to see this reviewed and merged!

@morgante
Copy link
Contributor

@ericyz Could you rebase/resolve conflicts?

@bharathkkb
Copy link
Member

Superseded by #729

@bharathkkb bharathkkb closed this Nov 2, 2020
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.

None yet

4 participants