-
Notifications
You must be signed in to change notification settings - Fork 828
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
Add a script to recreate the "main" GCP project #266
Conversation
HOLD until we are really confident in this. |
I have run this and created kubernetes-public-thockin1 which seems to parallel kubernetes-public for everything I have looked at |
2783b56
to
3173403
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - two suggestions but nothing important.
k8s.gcr.io/ensure-main-project.sh
Outdated
--member "group:${CLUSTER_ADMINS_GROUP}" \ | ||
--role roles/container.admin | ||
if ! gcloud --project "${PROJECT}" iam roles describe ServiceAccountLister >/dev/null 2>&1; then | ||
# Don't use `yes` here, it causes SIGPIPE -> pipefail -> errexit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does --quiet
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does! TIL
__EOF__ | ||
|
||
FINAL=$(tempfile -p k8s-infra-bq-access-new) | ||
jq -s '.[0].access + .[1].access | { access: . }' "${CUR}" "${ENSURE}" > "${FINAL}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment would be great here to explain what is happening! We're merging the existing and new permissions? (And meta: any reason not just to set the permissions explicitly so we do delete any unwanted permissions?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thockin I don't see the comment. forgot to push?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I swear I wrote a comment. Added now.
I think this looks right. With DNS excluded, the risk is primarily for the k8s cluster(s) we are running? (I am assuming there's nothing that important in gcr or gcs yet)? Are you planning on actually deleting the project and recreating it? /lgtm (edit: s/gce/gcs/g) |
3173403
to
2b35e63
Compare
New changes are detected. LGTM label has been removed. |
new push is up I do not plan to nuke the project, just trying to make sure everything we rely on is recreatable and explainable. |
2b35e63
to
cc69728
Compare
Removed the hold. PTAL. I have a followup to move this all to a new home and do some cleanups. |
cc69728
to
ae00197
Compare
ping |
LGTM, still missing a comment requested by @justinsb |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This covers everything but DNS, which I will do separately. When we have clusters, some of this might move out to a clusters script. All these scripts need a new dir, rather than GCR :)
ae00197
to
b0e139a
Compare
This covers everything but DNS, which I will do separately.
When we have clusters, some of this might move out to a clusters script.
All these scripts need a new dir, rather than GCR :)