Skip to content

Conversation

tasdomas
Copy link
Contributor

@tasdomas tasdomas commented Sep 6, 2022

This PR contains a few other changes as well:

  • generic rclone connection string generation
  • using rclone to check that remote storage is accessible
  • updates to the aws and gcp providers to use the same rclone utilities.

@tasdomas tasdomas temporarily deployed to automatic September 6, 2022 13:05 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 6, 2022 13:06 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 6, 2022 13:06 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 6, 2022 13:06 Inactive
@tasdomas tasdomas requested a deployment to automatic September 6, 2022 19:17 Abandoned
@tasdomas tasdomas requested a deployment to automatic September 6, 2022 19:19 Abandoned
@tasdomas tasdomas requested a deployment to automatic September 6, 2022 19:19 Abandoned
@tasdomas tasdomas requested a deployment to automatic September 6, 2022 19:19 Abandoned
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented Sep 7, 2022

@tasdomas, would you mind setting the base branch of #660 (this) to #651 so I don't have to tweak GitHub filters too much to review only the changes it introduces?

@tasdomas
Copy link
Contributor Author

tasdomas commented Sep 7, 2022

@tasdomas, would you mind setting the base branch of #660 (this) to #651 so I don't have to tweak GitHub filters too much to review only the changes it introduces?

I was thinking that we'd land #651 first and then move on to this.

@tasdomas tasdomas force-pushed the d012-bucket-reuse-azure branch from f149c7b to aa5d79c Compare September 8, 2022 09:20
@tasdomas tasdomas temporarily deployed to automatic September 8, 2022 09:20 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 8, 2022 09:20 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 8, 2022 09:20 Inactive
…re data source.

Use common.RemoteStorage to initialize the data sources.
Using rclone to verify storage during Read.
Remove aws s3 client mocks and tests that rely on them.
@tasdomas tasdomas temporarily deployed to automatic September 8, 2022 11:53 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 8, 2022 11:53 Inactive
@tasdomas
Copy link
Contributor Author

@tasdomas it looks like #651 is merged, Is this ready for review now?

It is, yes

@tasdomas tasdomas temporarily deployed to automatic September 10, 2022 06:11 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 10, 2022 06:12 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 10, 2022 06:12 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 10, 2022 06:12 Inactive
@tasdomas tasdomas temporarily deployed to automatic September 10, 2022 06:12 Inactive
@tasdomas tasdomas requested review from 0x2b3bfa0 and dacbd September 12, 2022 09:54
)

// NewExistingS3Bucket returns a new data source refering to a pre-allocated
// S3 bucket.
func NewExistingS3Bucket(client S3Client, credentials aws.Credentials, id string, region string, path string) *ExistingS3Bucket {
func NewExistingS3Bucket(credentials aws.Credentials, storageParams common.RemoteStorage) *ExistingS3Bucket {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing common.RemoteStorage to create a reference to an existing s3 bucket, instead of individual params.

return common.NotFoundError
}
return err
err := machine.CheckStorage(ctx, b.connection())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using rclone to verify bucket access (this will be done across all providers, sans k8s)

@@ -1,5 +0,0 @@
package mocks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By moving to using rclone to validate bucket access, these mocks are no longer useful. I'll probably revisit the idea at some point.

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

I still believe that it would be better to treat and expose rclone connection strings as such to users instead of building them internally, but that's the kind of review comment that makes more sense on the whole feature branch.

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

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

I guess we can merge it, before it breathes too much and leave some reviews on the overall approach for the feature branch.

Copy link
Contributor

@dacbd dacbd left a comment

Choose a reason for hiding this comment

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

LGTM, I had some az credential issues but that is unrelated 🙃

@tasdomas tasdomas merged commit dc3374d into feature-bucket-reuse Sep 14, 2022
@tasdomas tasdomas deleted the d012-bucket-reuse-azure branch September 14, 2022 16:14
@casperdcl casperdcl added cloud-az Microsoft Azure technical-debt Refactoring, linting & tidying labels Sep 23, 2022
tasdomas pushed a commit that referenced this pull request Oct 21, 2022
* Support for existing storage containers in aws and gcp. (#651)

* Update config schema.

* AWS support for existing S3 buckets.

* Existing bucket support for gcp.

* Add mocks and tests to existing bucket resource in aws.

* Update docs with new pre-allocated container fields.

* Support using pre-allocated blob containers in azure. (#660)

* AWS support for existing S3 buckets.

* Existing bucket support for gcp.

* Fix subdirectory in rclone remote.

* Blob container generates the rclone connection string.

* Introduce a type for generating rclone connection strings.

* Azure support for reusable blob containers.

* Update docs.

* Fix path prefix.

* Initialize s3 existing bucket with RemoteStorage struct.

* Update gcp and aws existing bucket data sources to align with the azure data source.

Use common.RemoteStorage to initialize the data sources.
Using rclone to verify storage during Read.
Remove aws s3 client mocks and tests that rely on them.

* Fix comment.

* K8s support for specifying an existing persistent volume claim (#661)

* K8s support for specifying an existing persistent volume claim.

Co-authored-by: Helio Machado <[email protected]>

* Update use of Identifier struct.

* Combine container and container_path config keys.

* Update docs.

* Use split function from rclone.

Co-authored-by: Helio Machado <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cloud-az Microsoft Azure technical-debt Refactoring, linting & tidying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants