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

Add support for primary store of Azure blob #2788

Merged
merged 14 commits into from
Aug 20, 2019

Conversation

Lucretius
Copy link

This PR adds support for storing logs in Azure blob similar to how they can be stored in S3. The following environment variables are added to support this:

DRONE_AZURE_BLOB_CONTAINER_NAME
DRONE_AZURE_STORAGE_ACCOUNT_NAME
DRONE_AZURE_STORAGE_ACCESS_KEY

If DRONE_AZURE_BLOB_CONTAINER_NAME is set, the logStore will be set to Azure.

I tested this out running Drone via ngrok on my system and saw the logs appearing in the desired Azure storage account container.

@CLAassistant
Copy link

CLAassistant commented Aug 14, 2019

CLA assistant check
All committers have signed the CLA.

@tboerger
Copy link

The question is if this is really something that can be maintained/supported. An alternative could be to just launch a Minio azure gateway beside Drone, and just configure Minio as an regular S3 target.

@@ -81,17 +81,26 @@ func provideBuildStore(db *db.DB) core.BuildStore {
// provideLogStore is a Wire provider function that provides a
// log datastore, configured from the environment.
func provideLogStore(db *db.DB, config config.Config) core.LogStore {
if config.S3.Bucket == "" {
if config.S3.Bucket == "" && config.AzureBlob.ContainerName == "" {

Choose a reason for hiding this comment

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

can we cleanup the if / else statements here to improve readability?

if config.S3.Bucket != "" {
  ...
  return logs.NewCombined(p, s)
}
if config.AzureBlob.ContainerName != "" {
  ...
  return logs.NewCombined(p, s)
}
return logs.New(db)

go.mod Outdated
@@ -3,6 +3,9 @@ module github.com/drone/drone
require (
docker.io/go-docker v1.0.0
github.com/99designs/httpsignatures-go v0.0.0-20170731043157-88528bf4ca7e
github.com/Azure/azure-pipeline-go v0.2.1

Choose a reason for hiding this comment

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

is this needed?

@bradrydzewski
Copy link

bradrydzewski commented Aug 14, 2019

the only concern I have is that this seems to be adding a lot of transitive dependencies to the project which seems a bit odd. How did you update the dependencies? can these be pruned?

the azure-storage-blob-go library should only add a few dependencies:
https://github.com/Azure/azure-storage-blob-go/blob/master/go.sum

@Lucretius
Copy link
Author

Appreciate the quick feedback guys.

the only concern I have is that this seems to be adding a lot of transitive dependencies to the project which seems a bit odd. How did you update the dependencies? can these be pruned?

the azure-storage-blob-go library should only add a few dependencies:
https://github.com/Azure/azure-storage-blob-go/blob/master/go.sum

VSCode is handling the imports for me automatically - I guess it went ham and updated everything as well. The reason that pipeline thing was there was because I had at some point relied upon the actual struct in the code and looks like it never got cleaned up.
Let me try restoring to the original go.mod/go.sum and manually adding the dependency and seeing if that makes things better.

The question is if this is really something that can be maintained/supported. An alternative could be to just launch a Minio azure gateway beside Drone, and just configure Minio as an regular S3 target.

Definitely a reasonable concern. I don't think the blob store necessarily adds too much to the codebase (once I clean up go.mod/go.sum) so I figured it would be simple enough to tack on, but I do get the bigger picture concern that each cloud provider has an "S3-like" storage capability, and that this repo could end up with a large amount of cloud-provider specific logging code if we set the precedent now. I could see moving the cloud-specific logger code outside of this codebase using some sort of a plugin-based approach. The minio gateway looks to be a pretty viable option too.

@bradrydzewski
Copy link

bradrydzewski commented Aug 14, 2019

Let me try restoring to the original go.mod/go.sum and manually adding the dependency and seeing if that makes things better.

thanks, I see the updated go.sum and it definitely looks much better

I could see moving the cloud-specific logger code outside of this codebase using some sort of a plugin-based approach. The minio gateway looks to be a pretty viable option too.

there is no hard rule, and we certainly do not want to support every possible object storage out there. so the question is where do we draw the line? There is no good answer to this question, however, my thought is that we support the major cloud providers (aws, gcp, azure) and we support s3 protocol and that is where we draw the line.

it is also possible we create some sort of pluggable storage in the future, but until such a time, I can get on board with merging azure support mainline.

@Lucretius
Copy link
Author

Lucretius commented Aug 14, 2019

I think a cut-off at the three major providers makes sense (I would be glad to take a look at doing the implementation for GCP as well). I will test this code again tonight to confirm everything is working as expected. Aside from that is there anything else you would like from me as far as this PR is concerned?

EDIT: Everything appears to be working. Checked again this morning, logs are being written and Drone can read them just fine.

@bradrydzewski bradrydzewski merged commit d581dab into harness:master Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants