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

Script adding a GCR staging repo #186

Merged
merged 3 commits into from
Feb 20, 2019

Conversation

thockin
Copy link
Member

@thockin thockin commented Feb 11, 2019

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 11, 2019
@k8s-ci-robot k8s-ci-robot requested review from dims and nikhita February 11, 2019 22:44
@thockin
Copy link
Member Author

thockin commented Feb 11, 2019

As promised on Friday, the script.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 11, 2019
@dims
Copy link
Member

dims commented Feb 11, 2019

Related to #158

gcloud \
projects add-iam-policy-binding "${PROJECT}" \
--member "group:${ADMINS}" \
--role roles/viewer
Copy link
Contributor

Choose a reason for hiding this comment

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

do we just want admins as 'viewers'? what about 'owners'?

Copy link
Member Author

Choose a reason for hiding this comment

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

They only need project viewer for the UI to work. They have bucket admin for the specific buckets.

We're trying to keep untra-limited perms when possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

who is going to manage the projects? just who create them?

Copy link
Member Author

Choose a reason for hiding this comment

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

the set of people who can create projects is very small and should, itself be governed by a googlegroup

# Grant repo writers access to write.
color 6 "Granting bucket objectAdmin to ${WRITERS}"
gsutil iam ch "group:${WRITERS}:objectAdmin" "gs://${BUCKET}"
color 6 "Granting bucket legacyBucketReader to ${WRITERS}"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need 'legacyBucketReader'?
are we going to store GCS buckets as well or just GCR?

Copy link
Member Author

Choose a reason for hiding this comment

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

without buckets.list it didn't work, and there doesn't seem to be a role that includes that permission that isn't "legacy" (and I didn't want to make a custom role, though we could, I guess...)

Copy link
Contributor

Choose a reason for hiding this comment

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

you are right, the other alternative is 'roles/storage.admin'

Copy link
Member Author

Choose a reason for hiding this comment

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

which is too broad

@javier-b-perez
Copy link
Contributor

LGTM

as side note, we should also document somewhere who is going to own the groups like 'k8s-infra-gcr-staging-${REPO}@googlegroups.com', probably another google group?

@dims
Copy link
Member

dims commented Feb 11, 2019

/assign @justinsb

Justin, can you please peek?

@thockin
Copy link
Member Author

thockin commented Feb 11, 2019

I am working on a quick followup commit to age-out old data

@thockin
Copy link
Member Author

thockin commented Feb 11, 2019

PTAL at last commit, too

BILLING="018801-93540E-22A20E"

# Make the project, if needed
if ! gcloud projects describe "${PROJECT}" >/dev/null 2>&1; then
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this project name happens to be an unrelated 3rd party?

Copy link
Member Author

Choose a reason for hiding this comment

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

You won't be able to list those. I don't see a way to only list projects under a specific org - do you know of one? Assuming you have a project with this name pattern in your scope, something else will fail. Or if you have project admin, maybe it won't fail.

We could require this create to happen, which makes the scripts not re-runnable. We could add flags to say --project-exists or something. Is that worth the effort?

docker tag k8s.gcr.io/pause "gcr.io/${PROJECT}/pause"
docker push "gcr.io/${PROJECT}/pause"
gcloud --project "${PROJECT}" \
container images delete --quiet "gcr.io/${PROJECT}/pause:latest"
Copy link
Member

Choose a reason for hiding this comment

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

Scary! Worth tagging gcr.io/${PROJECT}/ceci-nest-pas-une-image or some other thing that isn't important?

Copy link
Member Author

Choose a reason for hiding this comment

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

hahah, sure. In next push

@justinsb
Copy link
Member

Two paranoid suggestions, but lgtm

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2019
@thockin
Copy link
Member Author

thockin commented Feb 14, 2019

New push is up with image name change. Other is unresolved. Launch and iterate?

@dims
Copy link
Member

dims commented Feb 14, 2019

thanks for adding the age-out. +1 to launch/iterate

/approve

leaving /lgtm for @justinsb

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2019
@thockin
Copy link
Member Author

thockin commented Feb 14, 2019 via email

@thockin
Copy link
Member Author

thockin commented Feb 14, 2019 via email

@thockin
Copy link
Member Author

thockin commented Feb 20, 2019

ping for lgtm

@dims
Copy link
Member

dims commented Feb 20, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2019
@k8s-ci-robot k8s-ci-robot merged commit f8ec9c0 into kubernetes:master Feb 20, 2019
@thockin thockin deleted the script-gcr-staging branch November 1, 2019 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants