Skip to content

Properly fix acquire_cluster_admin_role() #87

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.

Sign up for GitHub

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
adrcunha merged 1 commit into from
Aug 22, 2018
Merged

Properly fix acquire_cluster_admin_role() #87

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion scripts/library.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,16 @@ function acquire_cluster_admin_role() {
# might not have the necessary permission.
local password=$(gcloud --format="value(masterAuth.password)" \
container clusters describe $2 --zone=$3)
kubectl config set-credentials cluster-admin --username=admin --password=${password}
kubectl config set-credentials cluster-admin \
--username=admin --password=${password}
Copy link
Copy Markdown
Contributor

@jessiezcc jessiezcc Aug 22, 2018

Choose a reason for hiding this comment

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

so this will break when prow upgrades to 1.11?

Copy link
Copy Markdown
Contributor Author

@adrcunha adrcunha Aug 22, 2018

Choose a reason for hiding this comment

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

No, it's backwards compatible.

Copy link
Copy Markdown
Contributor

@jessiezcc jessiezcc Aug 22, 2018

Choose a reason for hiding this comment

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

in description, it says 1.11 no longer recognizes username and password. then set-credentials won't work, isn't it?

Copy link
Copy Markdown
Contributor Author

@adrcunha adrcunha Aug 22, 2018

Choose a reason for hiding this comment

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

No. It doesn't recognize them as global flags.

Copy link
Copy Markdown
Contributor Author

@adrcunha adrcunha Aug 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

(compare the original code with the current code; updated description again)

kubectl config set-context $(kubectl config current-context) \
Copy link
Copy Markdown
Contributor

@jessiezcc jessiezcc Aug 22, 2018

Choose a reason for hiding this comment

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

what's the new context?

Copy link
Copy Markdown
Contributor Author

@adrcunha adrcunha Aug 22, 2018

Choose a reason for hiding this comment

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

The same context.

Copy link
Copy Markdown
Contributor

@jessiezcc jessiezcc Aug 22, 2018

Choose a reason for hiding this comment

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

ok, I got "new context" from description. Looks like u already updated it.

Copy link
Copy Markdown
Contributor Author

@adrcunha adrcunha Aug 22, 2018

Choose a reason for hiding this comment

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

Yes, there was indeed a mixup, and I fixed the description to reflect the implementation after I saw your comment. Originally I used a new context, but that was unnecessary and just added more complexity.

--user=cluster-admin
kubectl create clusterrolebinding cluster-admin-binding \
--clusterrole=cluster-admin \
--user=$1
# Reset back to the default account
gcloud container clusters get-credentials \
$2 --zone=$3 --project $(gcloud config get-value project)
}

# Runs a go test and generate a junit summary through bazel.
Expand Down