Add block adapters unit tests #5317

N-o-Z · 2023-02-22T18:30:00Z

Linked Issue

Change Description

Background

Adding generic unit test for block adapters, in order to align functionality and behavior
For the scope of this PR, added testing for the majority of the adapter interface, and enabled unit testing for local and Azure block adapters

Breaking Change?

No

itaiad200

I was skeptical at first when you came up with this idea.. Seeing how easy it is to add adapter tests and the guarantees they grant us, it really is a great addition to our coverage.

itaiad200 · 2023-02-22T19:36:46Z

pkg/block/azure/adapter.go

@@ -325,7 +325,7 @@ func (a *Adapter) Walk(ctx context.Context, walkOpt block.WalkOpts, walkFn block
 					return err
 				}
 			}
-			if marker = resp.NextMarker; marker == nil {
+			if marker = resp.NextMarker; marker == nil || *marker == "" {


Are both nil and empty string really an option? Yikes.

Yes - unfortunately verified with real adapter

itaiad200 · 2023-02-22T19:38:11Z

pkg/block/params/block.go

@@ -46,4 +46,5 @@ type Azure struct {
 	StorageAccessKey string
 	TryTimeout       time.Duration
 	PreSignedExpiry  time.Duration
+	Url              *string


What is this?

This is a config parameter for testing purposes to provide a custom URL to override the default URL template - added comment

Maybe change to Endpoint?

itaiad200 · 2023-02-22T19:38:54Z

pkg/block/mem/adapter_test.go

-	var params []func(*mem.Adapter)
-	adapterTest.TestAdapter(t, block.BlockstoreTypeMem, params)
-}
+// TODO (niro): Need to enable


A marker so you won't forget

itaiad200 · 2023-02-22T19:42:55Z

pkg/block/azure/main_test.go

+
+func runAzurite(dockerPool *dockertest.Pool) (string, func()) {
+	ctx := context.Background()
+	resource, err := dockerPool.Run("mcr.microsoft.com/azure-storage/azurite", "latest", []string{


Pin a version or wake up one day to find out we're not compatible with latest no more :)

itaiad200 · 2023-02-22T19:45:29Z

pkg/block/azure/main_test.go

+	if err != nil {
+		panic(err)
+	}
+	hosts.AddHost("127.0.0.1", accountHost)


Are we overriding the host machine /etc/hosts file? If so, I'm worried about this one. is there any other way that doesn't require overriding the machine /etc/hosts? Test can crash half way through leaving garbage in that file..

By default the emulator uses an IP style URL where the account name is part of the pah. This breaks our code due to how we parse the URL as we expect to account to be the domain.
So we need to add a host to our local IP which maintains the same structure as the production service URL.
This was the best option I found to do that - I'm open to better suggestions

Gotcha, so the real problem is that we don't support accounts as part of the path.

@nopcoder suggested that we'll leverage s3.local.lakefs.io somehow. Is it possible to use a real DNS record that will point to localhost?

The domain is comprised from the account name - which is dynamically determined. How do you suggest to deal with that?

We add a DNS record *.local.azurite.lakefs.io (or something similar) that points to 127.0.0.1. Then for every account name xx we will use the host name xx.local.azurite.lakefs.io, thus avoiding hosts file modifications.

The usual way is to spin up everything (test and Azurite) inside a single docker-compose environment.

On the one hand, users cannot edit hosts lookup -- this should never work.

In the past we had a DNS record for ease of deployment of local S3 gateways with host-based addressing. It makes a log of sense for quick-starting users. But using a DNS record here hurts in other ways, it means "unit" tests require Internet connectivity.

As an alternative, you can override net.DefaultResolver with something that resolves *.azurite.example.net but falls back on the original DefaultResolver for everything else.

itaiad200 · 2023-02-22T19:48:15Z

pkg/block/azure/main_test.go

+	client, err := service.NewClientWithSharedKeyCredential(url, cred, nil)
+	if err != nil {
+		panic(err)
+	}
+
+	// Check connectivity with containerName
+	_, err = client.GetAccountInfo(ctx, nil)
+	if err != nil {
+		panic(err)
+	}


I think the below lines cover connectivity check

pkg/block/azure/main_test.go

itaiad200 · 2023-02-22T19:50:23Z

pkg/block/blocktest/adapter.go

+	obj := block.ObjectPointer{
+		StorageNamespace: storageNamespace,
+		Identifier:       contents,
+		IdentifierType:   block.IdentifierTypeRelative,


Checking Full identifiers is critical too.

itaiad200 · 2023-02-22T19:52:22Z

pkg/block/blocktest/adapter.go

+		{"simple", "abc", []string{"one ", "two ", "three"}},
+		{"nested", "foo/bar", []string{"one ", "two ", "three"}},


This will fail for some adapters. For example, S3 forces all parts except the last to be at least 5MB.

Understand - lets handle this when it becomes an issue?

itaiad200 · 2023-02-22T19:53:04Z

pkg/block/blocktest/adapter.go

+	return tree
+}
+
+func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageNamespace string) {


Aborting multipart uploads is also important.
It can be added in a different PR.

Yes - not everything was covered in this PR, I'll open a separate issue

N-o-Z · 2023-02-23T09:05:27Z

I was skeptical at first when you came up with this idea.. Seeing how easy it is to add adapter tests and the guarantees they grant us, it really is a great addition to our coverage. !

Credit goes to @nopcoder 😄

…it-tests # Conflicts: # pkg/block/params/block.go # pkg/config/config.go

itaiad200

Requesting changes to consider alternatives to hosts file modifications.

pkg/block/local/adapter_test.go

pkg/block/test/adapter.go

arielshaqed

Minor comment about encapsulating tests from The Internet as far as possible.

arielshaqed · 2023-02-26T07:09:48Z

pkg/block/azure/main_test.go

+	if err != nil {
+		panic(err)
+	}
+	hosts.AddHost("127.0.0.1", accountHost)


The usual way is to spin up everything (test and Azurite) inside a single docker-compose environment.

On the one hand, users cannot edit hosts lookup -- this should never work.

In the past we had a DNS record for ease of deployment of local S3 gateways with host-based addressing. It makes a log of sense for quick-starting users. But using a DNS record here hurts in other ways, it means "unit" tests require Internet connectivity.

arielshaqed · 2023-02-26T07:13:09Z

pkg/block/azure/main_test.go

+	if err != nil {
+		panic(err)
+	}
+	hosts.AddHost("127.0.0.1", accountHost)


As an alternative, you can override net.DefaultResolver with something that resolves *.azurite.example.net but falls back on the original DefaultResolver for everything else.

itaiad200

Thanks for applying the hosts change!

arielshaqed

Nice! But I worry about binding port 53 in particular, and any constant port in general. Tests should bind only ephemeral ports, or they risk unpleasant surprises (PC firewall popups, outright refusal on POSIX, inability to run the test twice concurrently, etc.).

Can we

arielshaqed · 2023-02-27T13:15:46Z

pkg/block/azure/main_test.go

+	}
+
+	srv := &dns.Server{
+		Addr:    ":53",


I think binds to a specific port on the host machine. But that port may be occupied, maybe someone runs dnsmasq! Also because this uses a port <1024 it has to run as root on some systems.

Consider binding only on "127.0.0.1:0" or similar, and then querying the server which ephemeral port it received. Not sure you can do it with this DNS server package though. I think that this dns.Server might be able to bind an ephemeral port if you configure it with Addr: "127.0.0.1:0", and then fetch that port using net.SplitHostPort(server.Listener.Addr().String()).

# Conflicts: # pkg/block/azure/adapter.go # pkg/block/local/adapter.go # pkg/block/local/adapter_test.go # pkg/block/mem/adapter_test.go

N-o-Z · 2023-03-02T15:59:13Z

Had to do a big refactor due to removal of the Walk function in master.
Added fixes for namespace resolution and other

arielshaqed

Thanks!

One thing missing from the tests that is particularly relevant to the local block adapter in production is testing that path traversal doesn't work.

Paths like a/b/./c must not be the same as a/b/c (maybe they should error out).
Paths like a/b/../b/c must not be the same as a/b/c (maybe they should error out).
Paths like a///b must be handled correctly or error out (POSIX will treat them as a/b).
Paths like namespace/prefix/../../../../../../../etc/passwd that escape out of namespace/prefix must error out.
Paths like namespace/prefix/a/b/\0/c where \0 is the NUL char (byte 0) must either error out or be interpreted the same on POSIX filesystems in local mode and on lakeFS.

Another thing to test (which might be considerably more difficult!) is that POSIX filesystems use the same sort as

arielshaqed · 2023-03-06T09:31:31Z

pkg/block/azure/client_cache.go

+	// For testing purposes - override default url template
+	if params.URL != nil {


If this really is only for testing, consider renaming the configuration option to something like url_for_testing. Comments are too easy to ignore.

arielshaqed · 2023-03-06T09:34:52Z

pkg/block/blocktest/adapter.go

+	"github.com/treeverse/lakefs/pkg/ingest/store"
+)
+
+func TestAdapter(t *testing.T, adapter block.Adapter, storageNamespace, externalPath string) {


This function probably should not be named Test*, that's the name of a test function. Maybe something like VerifyAdapter?

Maybe I'm missing something - but this is a test function

Not really: it's a helper function for writing test functions as described in package testing.

Final counterpoint from me on this topic: suppose TestAdapter included reading the config and therefore its callers would set all values from the environment. It would need to have signature TestAdapter(t *testing.T) -- and then package testing would try to run it! In that case you would need to rename the func. So naming a function Test* is confusing when you expect it not to run automagically.

pkg/block/blocktest/adapter.go

arielshaqed · 2023-03-07T10:18:06Z

pkg/block/local/walker.go

@@ -81,7 +83,7 @@ func (l *LocalWalker) Walk(_ context.Context, storageURI *url.URL, options WalkO
 			}
 			const dirPerm = 0o755
 			_ = os.MkdirAll(l.cacheLocation, dirPerm)
-			cacheName := filepath.Join(l.cacheLocation, nanoid.Must()+"-import.json")
+			cacheName := filepath.Join(l.cacheLocation, gonanoid.Must()+"-import.json")


I don't understand. Shouldn't this live under the system temp file area, and be handled that way?

local block adapter is constrained to the provided path and repo namespace (and configured allowed external prefixes).
We cannot assume the user allows access to the temp path.

Import assume local adapter is configured to a shared location - so cache folder must be under the adapter's path.

This might be quite surprising or confusing to users. We should take care to document this.

arielshaqed · 2023-03-07T10:31:11Z

pkg/block/local/walker.go

@@ -81,7 +83,7 @@ func (l *LocalWalker) Walk(_ context.Context, storageURI *url.URL, options WalkO
 			}
 			const dirPerm = 0o755
 			_ = os.MkdirAll(l.cacheLocation, dirPerm)
-			cacheName := filepath.Join(l.cacheLocation, nanoid.Must()+"-import.json")
+			cacheName := filepath.Join(l.cacheLocation, gonanoid.Must()+"-import.json")
 			const cachePerm = 0o644
 			if err := os.WriteFile(cacheName, jsonData, cachePerm); err != nil {


What deletes this file? Especially consider what happens during failure: if the file is not deleted, the user will try again... and nothing will release this file. Especially especially consider what happens during unexpected failure.

The usual way to handle temporary files in POSIX is:

Create a temporary file (creat).

Delete it.

Write whatever contents you need.

lseek back to the start of the file.

Read whatever you need.

...

close the file.

POSIX filesystems explicitly support this mode: A deleted file has no name, and is deleted when no open file descriptors remain on it. This pattern is already implemented in lakeFS.

In case a continuation token (file) is required for the next read, we cannot deleted the file.
The write and read will happen in a different context, and therefore we cannot use the pattern you described.
I added a remove in case of an error.
I agree we do need to have a cleanup mechanism for stale continuation token files, but not quite sure how we should do it - in any case I don't think this should be under the scope of this PR.
I'll open a new issue for it.

Understood that this is not part of this PR. Note also that keeping temporary files in the permanent storage area (previous comment) makes this even more confusing.

nopcoder

Looks great @N-o-Z - added my comments/concerns

pkg/block/mem/adapter.go

nopcoder · 2023-03-06T15:47:23Z

pkg/block/namespace.go

-		Prefix:           key.Key,
-	}, nil
-}
-
 func ResolveNamespace(defaultNamespace, key string, identifierType IdentifierType) (QualifiedKey, error) {


block.ResolveNamespace - think we should name it a bit different than the name we have on the interface or name the method on the interface differently.
It can be that block.ResolveNamespace -> DefaultResolveNamespace
Or .ResolveNamespace -> ResovleQualifiedKey
I'm trying to make it clear to the caller what the code will return for each case.

nopcoder · 2023-03-06T16:03:05Z

pkg/block/namespace.go

+// QK - QualifiedKey interface
+type QK interface {
+	Format() string
+	GetStorageType() StorageType
+	GetStorageNamespace() string
+	GetKey() string


Think this one should be named QualifiedKey as the block adapter interface will work only with the interface.

We can name the block level implementation as Common/GeneralQualifiedKey and the local one as LocalQualifiedKey. Need to identify the difference between the block level qualified key type and functions vs the interface which provided by our interface - the application level would like to work with the block adapter level type which is QualifiedKey interface.

Naming GetXXX -> XXX - I understand that as long as the implementation provides the same property it can conflict - but maybe we can find different names for struct/interface so we can keep using getters without a Get prefix.

Suggested change

// QK - QualifiedKey interface

type QK interface {

Format() string

GetStorageType() StorageType

GetStorageNamespace() string

GetKey() string

type QualifiedKey interface {

Format() string

StorageType() StorageType

StorageNamespace() string

Key() string

nopcoder · 2023-03-06T16:51:00Z

pkg/block/namespace.go

+var (
+	ErrInvalidNamespace   = errors.New("invalid namespace")
+	ErrInvalidStorageType = errors.New("invalid storage type")
+)


Move to errors.go

Check maybe we can merge as both are used while we parse / build addresses - as "invalid address" with format that will render "namespace" and "storage type"

nopcoder · 2023-03-07T07:33:51Z

pkg/api/controller.go

@@ -302,7 +300,7 @@ func (c *Controller) GetPhysicalAddress(w http.ResponseWriter, r *http.Request,
 	}

 	address := c.PathProvider.NewPath()
-	qk, err := block.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative)
+	qk, err := c.Catalog.ResolveNamespace(repo.StorageNamespace, address, block.IdentifierTypeRelative)


I prefer not to have another ResolveNamespace on catalog too as already have another two.

nopcoder · 2023-03-07T08:40:21Z

pkg/ingest/store/factory.go

+	if f.params != nil {
+		localParams, err = f.params.BlockstoreLocalParams()
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	return local.NewLocalWalker(localParams), nil


The comment is related to the f.params - do we still require it as an interface?

Is it an option to call the builder without proving the right configuration for the adapter? maybe we should just return an error in f.params is nil.

nopcoder · 2023-03-07T08:41:28Z

pkg/ingest/store/factory.go

+	if s3EndpointURL != "" {
+		config = aws.Config{
+			Endpoint:         aws.String(s3EndpointURL),
+			Region:           aws.String("us-east-1"), // Needs region for validation as it is AWS client
+			S3ForcePathStyle: aws.Bool(true),
+		}


Why in this case we provide defaults? I prefer the caller to be explicit or let the adapter pull from the env.

pkg/block/transient/adapter.go

nopcoder · 2023-03-07T08:45:29Z

pkg/block/factory/build.go

@@ -128,7 +129,11 @@ func buildS3Adapter(statsCollector stats.Collector, params params.S3) (*s3a.Adap
 func BuildGSClient(ctx context.Context, params params.GS) (*storage.Client, error) {
 	var opts []option.ClientOption
 	if params.CredentialsFile != "" {
-		opts = append(opts, option.WithCredentialsFile(params.CredentialsFile))
+		credPath, err := homedir.Expand(params.CredentialsFile)


The config package usually expand the ~ to the user home dir.

nopcoder · 2023-03-07T12:14:09Z

pkg/block/blocktest/adapter.go

+	return tree
+}
+
+func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageNamespace string) {


Wonder how is this test associate to the real world case as multipart upload have minimum size of 5MB (unless it is the last part)

AFAIK this only applies to S3.
I assume that when we enable the unit tests for S3 we will need to modify the test

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

…o task/add-adapter-unit-tests

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

N-o-Z · 2023-03-15T12:41:47Z

Thanks!

One thing missing from the tests that is particularly relevant to the local block adapter in production is testing that path traversal doesn't work.

Paths like a/b/./c must not be the same as a/b/c (maybe they should error out).

Paths like a/b/../b/c must not be the same as a/b/c (maybe they should error out).

Paths like a///b must be handled correctly or error out (POSIX will treat them as a/b).

Paths like namespace/prefix/../../../../../../../etc/passwd that escape out of namespace/prefix must error out.

Paths like namespace/prefix/a/b/\0/c where \0 is the NUL char (byte 0) must either error out or be interpreted the same on POSIX filesystems in local mode and on lakeFS.

Another thing to test (which might be considerably more difficult!) is that POSIX filesystems use the same sort as

Thanks @arielshaqed

These are very important test cases and as I dived more into it I understand that this requires addressing in a separate PR.
I'll open a new issue to tackle both the questions raised and the testing for it.

pkg/block/blocktest/adapter.go

nopcoder

Thanks looks good. The only thing that hurts the "go" eyes is the multiple return values by extractParamsFromObj - looks we just need good struct name use per adapter to hold the values.

nopcoder · 2023-03-16T13:43:59Z

pkg/block/blocktest/adapter.go

@@ -228,6 +233,7 @@ func testAdapterMultipartUpload(t *testing.T, adapter block.Adapter, storageName
 }

 func testAdapterExists(t *testing.T, adapter block.Adapter, storageNamespace string) {
+	// TODO (niro): Test abs paths


Still relevant - alt open an issue

pkg/block/blocktest/adapter.go

nopcoder · 2023-03-16T15:01:42Z

pkg/block/local/walker.go

@@ -81,7 +83,7 @@ func (l *LocalWalker) Walk(_ context.Context, storageURI *url.URL, options WalkO
 			}
 			const dirPerm = 0o755
 			_ = os.MkdirAll(l.cacheLocation, dirPerm)
-			cacheName := filepath.Join(l.cacheLocation, nanoid.Must()+"-import.json")
+			cacheName := filepath.Join(l.cacheLocation, gonanoid.Must()+"-import.json")


Import assume local adapter is configured to a shared location - so cache folder must be under the adapter's path.

…it-tests # Conflicts: # pkg/api/controller.go

arielshaqed

Thanks, a lot of thought and effort are very evident!

Huge improvement, and probably needed since before I joined the company. Please make sure to link an issue for path traversal tests once we have one in a comment on this PR, so we don't lose track.

arielshaqed · 2023-03-19T08:08:59Z

pkg/block/blocktest/adapter.go

+	"github.com/treeverse/lakefs/pkg/ingest/store"
+)
+
+func TestAdapter(t *testing.T, adapter block.Adapter, storageNamespace, externalPath string) {


Not really: it's a helper function for writing test functions as described in package testing.

Final counterpoint from me on this topic: suppose TestAdapter included reading the config and therefore its callers would set all values from the environment. It would need to have signature TestAdapter(t *testing.T) -- and then package testing would try to run it! In that case you would need to rename the func. So naming a function Test* is confusing when you expect it not to run automagically.

arielshaqed · 2023-03-19T08:20:08Z

pkg/block/local/walker.go

@@ -81,7 +83,7 @@ func (l *LocalWalker) Walk(_ context.Context, storageURI *url.URL, options WalkO
 			}
 			const dirPerm = 0o755
 			_ = os.MkdirAll(l.cacheLocation, dirPerm)
-			cacheName := filepath.Join(l.cacheLocation, nanoid.Must()+"-import.json")
+			cacheName := filepath.Join(l.cacheLocation, gonanoid.Must()+"-import.json")


This might be quite surprising or confusing to users. We should take care to document this.

arielshaqed · 2023-03-19T08:21:57Z

pkg/block/local/walker.go

@@ -81,7 +83,7 @@ func (l *LocalWalker) Walk(_ context.Context, storageURI *url.URL, options WalkO
 			}
 			const dirPerm = 0o755
 			_ = os.MkdirAll(l.cacheLocation, dirPerm)
-			cacheName := filepath.Join(l.cacheLocation, nanoid.Must()+"-import.json")
+			cacheName := filepath.Join(l.cacheLocation, gonanoid.Must()+"-import.json")
 			const cachePerm = 0o644
 			if err := os.WriteFile(cacheName, jsonData, cachePerm); err != nil {


Understood that this is not part of this PR. Note also that keeping temporary files in the permanent storage area (previous comment) makes this even more confusing.

* Add block adapters unit tests * CR Fix 1 * Fix linter issue * Add fullpath tests * Ignore static check * CR Fixes * CR Fixes 2 * Rebase fixes * Fix lint error * Fix S3 Adapter namespace * Fix GS Adapter namespace * Fix ugc test * More fixes * Update pkg/block/blocktest/adapter.go Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io> * Update pkg/block/blocktest/adapter.go Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io> * Update pkg/block/mem/adapter.go Co-authored-by: Barak Amar <barak.amar@treeverse.io> * More fixes * Revert WriteRange * Add comment * More fixes * More fixes 2 --------- Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io> Co-authored-by: Barak Amar <barak.amar@treeverse.io>

Add block adapters unit tests

88b93b4

N-o-Z added exclude-changelog PR description should not be included in next release changelog team/versioning-engine Team versioning engine labels Feb 22, 2023

N-o-Z requested a review from a team February 22, 2023 18:30

N-o-Z self-assigned this Feb 22, 2023

itaiad200 requested changes Feb 22, 2023

View reviewed changes

N-o-Z added 5 commits February 23, 2023 11:29

CR Fix 1

b1866c0

Merge remote-tracking branch 'origin/master' into task/add-adapter-un…

4d5624c

…it-tests # Conflicts: # pkg/block/params/block.go # pkg/config/config.go

Fix linter issue

d6c3b15

Add fullpath tests

a7ebc0d

Ignore static check

50d17fc

N-o-Z requested a review from itaiad200 February 23, 2023 13:33

itaiad200 requested changes Feb 23, 2023

View reviewed changes

pkg/block/local/adapter_test.go Show resolved Hide resolved

pkg/block/test/adapter.go Outdated Show resolved Hide resolved

arielshaqed reviewed Feb 26, 2023

View reviewed changes

CR Fixes

561ed39

N-o-Z requested a review from itaiad200 February 26, 2023 17:58

itaiad200 approved these changes Feb 27, 2023

View reviewed changes

arielshaqed reviewed Feb 27, 2023

View reviewed changes

N-o-Z added 3 commits February 28, 2023 10:02

CR Fixes 2

79b6405

Merge branch 'master' into task/add-adapter-unit-tests

9af1b28

# Conflicts: # pkg/block/azure/adapter.go # pkg/block/local/adapter.go # pkg/block/local/adapter_test.go # pkg/block/mem/adapter_test.go

Rebase fixes

00d3c44

N-o-Z requested review from itaiad200, nopcoder and arielshaqed March 2, 2023 15:58

N-o-Z added 3 commits March 2, 2023 18:25

Fix lint error

fbb6794

Fix S3 Adapter namespace

17add7b

Fix GS Adapter namespace

52502cd

N-o-Z force-pushed the task/add-adapter-unit-tests branch from d69cad8 to d9bf3d6 Compare March 5, 2023 15:27

N-o-Z force-pushed the task/add-adapter-unit-tests branch from d9bf3d6 to 6043605 Compare March 6, 2023 07:56

Fix ugc test

a16fb04

N-o-Z force-pushed the task/add-adapter-unit-tests branch from 6043605 to a16fb04 Compare March 6, 2023 08:51

More fixes

e44d554

N-o-Z mentioned this pull request Mar 6, 2023

[Bug]: StageObject API for local block adapter doesn't check if local import is enabled #5361

Closed

arielshaqed requested changes Mar 7, 2023

View reviewed changes

nopcoder requested changes Mar 7, 2023

View reviewed changes

N-o-Z and others added 5 commits March 8, 2023 10:28

Merge branch 'master' into task/add-adapter-unit-tests

feaa0d5

Update pkg/block/blocktest/adapter.go

0eae4c6

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Merge remote-tracking branch 'origin/task/add-adapter-unit-tests' int…

78883f6

…o task/add-adapter-unit-tests

Update pkg/block/blocktest/adapter.go

6b1aab2

Co-authored-by: Ariel Shaqed (Scolnicov) <ariels@treeverse.io>

Update pkg/block/mem/adapter.go

52d9132

Co-authored-by: Barak Amar <barak.amar@treeverse.io>

N-o-Z added 2 commits March 15, 2023 18:47

More fixes

17cf898

Revert WriteRange

8968916

N-o-Z requested review from nopcoder and arielshaqed March 16, 2023 08:54

Add comment

376caf7

nopcoder reviewed Mar 16, 2023

View reviewed changes

pkg/block/blocktest/adapter.go Outdated Show resolved Hide resolved

nopcoder reviewed Mar 16, 2023

View reviewed changes

pkg/block/blocktest/adapter.go Outdated Show resolved Hide resolved

nopcoder approved these changes Mar 16, 2023

View reviewed changes

N-o-Z added 2 commits March 19, 2023 10:02

More fixes

5a0acd1

Merge remote-tracking branch 'origin/master' into task/add-adapter-un…

cac55e3

…it-tests # Conflicts: # pkg/api/controller.go

arielshaqed approved these changes Mar 19, 2023

View reviewed changes

N-o-Z mentioned this pull request Mar 19, 2023

Extend block adapter unit tests #5326

Closed

6 tasks

More fixes 2

76d13dd

N-o-Z enabled auto-merge (squash) March 19, 2023 09:29

N-o-Z merged commit d9cae2e into master Mar 19, 2023

N-o-Z deleted the task/add-adapter-unit-tests branch March 19, 2023 09:32

		{"simple", "abc", []string{"one ", "two ", "three"}},
		{"nested", "foo/bar", []string{"one ", "two ", "three"}},

		// For testing purposes - override default url template
		if params.URL != nil {

Add block adapters unit tests #5317

Add block adapters unit tests #5317

Conversation

N-o-Z commented Feb 22, 2023

Linked Issue

Change Description

Background

Breaking Change?

No

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented Feb 23, 2023

itaiad200 left a comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented Mar 2, 2023

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented Mar 15, 2023

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment