-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Google Cloud Builder #90
Conversation
return &GoogleCloudBuilder{cfg}, nil | ||
} | ||
|
||
func (cb *GoogleCloudBuilder) Run(out io.Writer, tagger tag.Tagger) (*BuildResult, error) { |
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.
Note: this doesn't actually use the tagger yet
StatusSuccess = "SUCCESS" | ||
|
||
// StatusFailure "FAILURE" - Build failed to complete successfully. | ||
StatusFailure = "FAILURE" |
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.
We can't get these from a generated proto anywhere?
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.
I couldn't find them anywhere. In the library they are still just hardcoded in the functions themselves
artifact.DockerfilePath = constants.DefaultDockerfilePath | ||
} | ||
logrus.Infof("Building artifact: %+v", artifact) | ||
cbBucket := fmt.Sprintf("%s%s", cb.GoogleCloudBuild.ProjectID, constants.GCSBucketSuffix) |
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.
I think there's a chance this bucket won't exist. It's created during the first cloud build call. We might need to create our own bucket for the user or prompt them...
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.
done
cbBucket := fmt.Sprintf("%s%s", cb.GoogleCloudBuild.ProjectID, constants.GCSBucketSuffix) | ||
buildObject := fmt.Sprintf("source/%s-%s.tar.gz", cb.GoogleCloudBuild.ProjectID, util.RandomID()) | ||
|
||
io.WriteString(out, fmt.Sprintf("Pushing code to gs://%s/%s\n", cbBucket, buildObject)) |
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.
fmt.Fprintf? Then you don't need to deal with an error.
return nil, errors.Wrap(err, "uploading source tarball") | ||
} | ||
var steps []*cloudbuild.BuildStep | ||
steps = append(steps, &cloudbuild.BuildStep{ |
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.
nit: inline this into the steps
declaration
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.
done
|
||
logrus.Debugf("get: bucket: %s object: %s offset: %d", bucket, objectName, offset) | ||
r, err := c.Bucket(bucket).Object(objectName).NewRangeReader(ctx, offset, -1) | ||
if err != nil { |
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.
I think there's a chance you'll get an error here if there weren't any logs written. I can show you a pointer on how gcloud handles this.
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.
yeah I kept getting 416s so i just ignored the error for now. I'll add in some status code switching
notes for myself:
should print codes:
200
206
should retry codes:
404 not found = logs not written yet
416 range not satisfiable = no new data
429 request throttle
503
anything else:
throw error
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.
done
return nil, fmt.Errorf("unknown status: %s", b.Status) | ||
} | ||
|
||
time.Sleep(time.Second) |
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.
nit: make this a constant somewhere.
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.
done
return r, nil | ||
} | ||
|
||
func createBucket(ctx context.Context, bucket, projectId string) error { |
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.
Have you verified this actually works? In particular I'm worried that if we create the bucket here, the cloudbuild service account may not have permissions to read it.
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.
did a minor refactor, but verified that it either creates a bucket if it doesnt exist or uses the existing bucket
return nil, errors.Wrap(err, "creating bucket if not exists") | ||
} | ||
|
||
io.WriteString(out, fmt.Sprintf("Pushing code to gs://%s/%s\n", cbBucket, buildObject)) |
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.
fmt.FPrintf
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.
done
return nil, errors.Wrapf(err, "getting build ID from op") | ||
} | ||
logsObject := fmt.Sprintf("log-%s.txt", remoteID) | ||
io.WriteString(out, fmt.Sprintf("Logs at available at \nhttps://console.cloud.google.com/m/cloudstorage/b/%s/o/%s\n", cbBucket, logsObject)) |
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.
fmt.Fprintf
defer c.Close() | ||
|
||
relDockerfilePath := filepath.Join(dockerCtx, dockerfilePath) | ||
w := c.Bucket(bucket).Object(objectName).NewWriter(ctx) |
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.
Need to check if the bucket is in the same project as the build.
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.
Looked over the API and I couldn't figure out how to do this. Any pointers?
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.
done
alright this should be ready for another review too |
Fixes #6