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.

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

Jump to bottom

Push to GCR on PRs #31

Merged
merged 11 commits into from
Jul 23, 2021
Merged

Push to GCR on PRs #31

merged 11 commits into from
Jul 23, 2021

Conversation

petechd
Copy link
Contributor

@petechd petechd commented Jul 21, 2021 •
edited
Loading

What is the context of this PR?

This adds new Push to GCR task to both pull-request and master workflows. New service account with Storage Legacy Bucket Reader(storage.buckets.get, storage.objects.list, storage.multipartUploads.list) and Storage Object Admin roles was created in the gcp project. This change requires two new secrets to be added to the repository: GCLOUD_SERVICE_KEY and GCLOUD_PROJECT_ID. This has been tested in this fork of launcher. Container Registry with pushed images can be found here.
EDIT: Runner and Launcher PRs are now pushing images tagged with branch name to our dev repository too.

How to review

  • Fork the repository
  • in gcp (your project) create new service account with Storage Legacy Bucket Reader and Storage Object Admin roles
  • add service account's json key as GCLOUD_SERVICE_KEY and your gcp project name as GCLOUD_PROJECT_ID to your forked repo secrets
  • create some pull requests or make some changes to master of your forked repository

@petechd petechd changed the title Add 'push to gcr' job to workflows Add push to gcr task to workflows Jul 21, 2021
@petechd
Copy link
Contributor Author

petechd commented Jul 21, 2021

Github Actions to push to ons-eq-gcr (3 SP)

MebinAbraham
MebinAbraham reviewed Jul 23, 2021
View reviewed changes
Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

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

Pushing on merges to master is needed for launcher, just not runner as there is no pipeline for pushing launcher.

@petechd petechd changed the title Add push to gcr task to workflows Push to GCR on PRs Jul 23, 2021
@petechd
Copy link
Contributor Author

petechd commented Jul 23, 2021

Pushing on merges to master is needed for launcher, just not runner as there is no pipeline for pushing launcher.

Added back now, thanks!

MebinAbraham
MebinAbraham previously approved these changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

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

Haven't explicitly tested master, but looks fine. PR push works as expected.

One other comment is that GCLOUD_PROJECT_ID could be renamed to GCR_PROJECT_ID for clarity. Change it if you/others agree.

@petechd
Copy link
Contributor Author

petechd commented Jul 23, 2021 •
edited
Loading

Haven't explicitly tested master, but looks fine. PR push works as expected.

One other comment is that GCLOUD_PROJECT_ID could be renamed to GCR_PROJECT_ID for clarity. Change it if you/others agree.

I tested pushing to master on my fork here , it pushes to pete-test-env-on-release container registry.
Will change project id now.

MebinAbraham
MebinAbraham reviewed Jul 23, 2021
View reviewed changes
Copy link
Contributor

@MebinAbraham MebinAbraham left a comment •
edited
Loading

Choose a reason for hiding this comment

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

My mistake, master changes can be removed. Just remembered, we introduced a push on merge pipeline for launcher last month.

@MebinAbraham MebinAbraham dismissed their stale review July 23, 2021 10:25

Awaiting changes

@petechd
Copy link
Contributor Author

petechd commented Jul 23, 2021

My mistake, master changes can be removed. Just remembered, we introduced a push on merge pipeline for launcher last month.

Yeah, I looked into concourse and saw it there but I thought that maybe I am wrong anyway..

@petechd petechd merged commit 42c47f6 into master Jul 23, 2021
@MebinAbraham MebinAbraham deleted the push-to-gcr branch July 23, 2021 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MebinAbraham MebinAbraham MebinAbraham approved these changes

@LJBabbage LJBabbage LJBabbage approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@petechd @LJBabbage @MebinAbraham