Add make targets to allow starting local cloud storage environment.

[milosgajdos]

This PR adds a couple of new make targets that let you spin a local cloud storage environment with min.io.

This lets you interact with an S3-compatible storage API. See the updated BUILDING.md document on how to get started.

Ideally, we integrate this into our CI pipeline soon, but there are still some failing tests in S3 driver that need addressing before we update our GHA workflows. This PR merely adds the environment setup targets.

Requirements:

• A docker compose compatible tool

[Jamstah]

I get that Go is capitalised, but then putting it in a codeblock makes it look like it should be a command. Perhaps italics, but not a codeblock :)

[Jamstah]

(Also should this and other general doc changes be in a separate pr really....)

[milosgajdos]

RE: code formatting - it was a styling change 😄 💅 I can revert

As for the general doc - I'm not sure, maybe? 🤔

[Jamstah]

Tbh I don't mind, it can piggy back

Giving it a go before a proper review.

[thaJeztah]

Yeah, it should probably be either "Go" (capitalised) or go (code block; the command), but not both

[milosgajdos]

Ok, made some updates to BUILDING.md 8e630ae

I also wonder if we should have DEVELOPING.md file that contains this local environment setup instead of putting the setup guide into the BUILDING.md

[thaJeztah]

Would it be worth creating Dockerfiles for these? (we can use build-arg for things we want to have customisable, such as the user and password, but perhaps that's not needed for this purpose. (maybe a MINIO_VERSION=... build-arg if you want to customise the version of the image that's used

[milosgajdos]

I think the only useful thing to justify the dedicated Dockerfile would be specifying the min.io version 🤔 but I'm willing to be persuaded otherwise 😄

[Jamstah]

I'd like to allow the user to pick a different compose tool, using docker compose is very tied to docker desktop, which is not good from a licensing perspective.

With my suggested changes I had it running with podman-compose on an selinux system quite happily.

[Jamstah]

I edited your description just FYI :)

[milosgajdos]

I'd like to allow the user to pick a different compose tool, using docker compose is very tied to docker desktop, which is not good from a licensing perspective

Agreed, should be addressed now

[Jamstah]

Generally LGTM, and all WFM.

I would consider naming of the make targets. Why cloud-storage and not minio? Should run-s3-tests not refer to minio, because that's the point?

Should start-cloud-storage be a pre-req for (dependency of) run-s3-tests (whatever we name them).

[milosgajdos]

I would consider naming of the make targets. Why cloud-storage and not minio?

The idea was that the underlying tool is irrelevant and we could swap it for something like localstack or jclouds or whatnot 🤷‍♂️

Should run-s3-tests not refer to minio, because that's the point?

I'd like to add run-gcp-tests there as GCS API is more or less compatible with S3 API IIRC (https://cloud.google.com/storage/docs/aws-simple-migration) so it'd be interesting to see if we can reuse min.io for GCS testing 🤔

Should start-cloud-storage be a pre-req for run-s3-tests (whatever we name them).

I believe it should, yeah 😬

[Jamstah]

Not using minio makes sense then, thanks.

To follow up on this one then:

Should run-s3-tests not refer to minio, because that's the point?

Should run-s3-tests be test-s3-using-local-cloud-storage?

Or even test-using-local-cloud-storage and run all the tests including enabling the S3 tests?

[milosgajdos]

Should run-s3-tests be test-s3-using-local-cloud-storage?

Tbh, I dont have a strong preference 🤔

Or even test-using-local-cloud-storage and run all the tests including enabling the S3 tests?

Ultimately I feel this would be the best thing to do. We should get the individual cloud storage drivers tests to pass using minio backing store and then we can tune things out in the Makefile and GHA 🤔

[milosgajdos]

PTAL @thaJeztah

[davidspek]

LGTM

[davidspek]

Awesome to have this included. Once it's merged we should setup a GitHub action so we can automatically run tests against MinIO for PRs.

[Jamstah]

Awesome to have this included. Once it's merged we should setup a GitHub action so we can automatically run tests against MinIO for PRs.

As soon as we make the tests pass :)

[milosgajdos]

Yes, what @Jamstah said. We also might wanna wait when minio/minio#18093 is released

[milosgajdos]

Do you want me to squash those 12 commits, @Jamstah 😅

[Jamstah]

I reviewed the code not the delivery, I don't mind either way :)

[milosgajdos]

We risk angering @thaJeztah though 😂 let's get his take

[Jamstah]

I thought it was something you also preferred?

And @davidspek wanted to start doing conventional commit in #4025

My take is I don't care unless the project cares. We haven't documented a project approach, or if we have, we haven't linked it properly from https://github.com/distribution/distribution/blob/main/CONTRIBUTING.md#contributing-code

[milosgajdos]

I dont think we have any policy, no. I'm happy to merge, I was just joking with @thaJeztah 😄

I think in past we sometimes asked folks to rebase, other times not (depending on the context i.e. are there any broken commits or not )...might be worth agreeing on something yeah 🤔

[thaJeztah]

I would've loved the commits to have been squashed before merging, as there's ... a lot, and most of them touch-ups 😓

[milosgajdos]

Yeah, I was thinking of that; we need some piece of doc/guideline in CONTRIBUTING.md and make sure to stick with it. In past, I've seen PRs merged with a similar number of commits many of which were also one-liners.

[thaJeztah]

It depends a bit; sometimes one-liner changes may still be a "unit of work" and go together with a commit message (which may be outlining why that one-line-change was needed); those can be very useful for "future reference".

For other changes, where code evolved during review (so follow-up commits are touching up "brand new code"), I don't think we should keep those, as they don't provide much information after code was merged (and it's better to have all of those in a single commit to see the change "as a whole").

That said; sometimes temporary commits can help during review ("to be squashed") to guide reviewers through changes made since their last review, and for "large units of work", it's worth splitting changes to smaller changes (but usually that is either "preparation" commits, or "refactoring" that needs to happen to assist the larger change).

We have some text in the moby repository about this (although it's not perfect - by far); https://github.com/moby/moby/blob/v24.0.6/CONTRIBUTING.md#successful-changes

Before you make a pull request, squash your commits into logical units of work using git rebase -i and git push -f. A logical unit of work is a consistent set of patches that should be reviewed together: for example, upgrading the version of a vendored dependency and taking advantage of its now available new feature constitute two separate units of work. Implementing a new function and calling it in another file constitute a single logical unit of work. The very high majority of submissions should have a single commit, so if in doubt: squash down to one.

After every commit, make sure the test suite passes. Include documentation changes in the same pull request so that a revert would remove all traces of the feature or fix.

[davidspek]

I was planning on getting started on the conventional commit enforcement tomorrow since it's also needed for the release process. It doesn't really help with the number of commits, but enforcing conventional messages does help with highlighting a commit should be squashed a bit.