Add symlink info to file metadata #153

k-ye · 2020-07-07T08:12:40Z

This PR just exposes the symlink information to filemeta.Metadata, without changing how the upper-level modules use it.

If the symlink is valid, it will fall through to the part of handling a normal file. This way, Digest, IsExecutable are still set.
Otherwise, symlink metadata's IsInvalid will be set to true, with Target being empty.

Issue: #146

ola-rozenfeld · 2020-07-07T17:48:14Z

go/pkg/filemetadata/filemetadata.go

+// SymlinkMetadata contains details if the given path is a symlink.
+type SymlinkMetadata struct {
+	Target    string
+	IsInvalid bool


nit: IsDangling is a more informative name, now that I think about it.

ola-rozenfeld · 2020-07-07T18:05:42Z

go/pkg/filemetadata/filemetadata_test.go

+func TestComputeSymlinks(t *testing.T) {
+	tests := []struct {
+		name   string
+		target interface{} // If unspecified, this is an invalid symlink


Some cleaner options for doing this:

What we usually do is make the test struct a union of the test parameters. There is nothing wrong with having an IsDirectory option, or an IsExecutable option that only applies to files and not directories.

If you really want to have a single field of variable type, the golang pattern is to create a common interface (say, CreateSymlink) and have your types implement it.
I slightly prefer option 2 in this case, because I suspect that if your createSymlink is split into two or three functions (you can create a different type for a dangling symlink), it will look much simpler.

Ah, actually i split this into three tests now. Sorry for mixing the logic before, hopefully it now gets simpler?

What we usually do is make the test struct a union of the test parameters.

Ack. I created testFileParams mostly because symlink tests also need this. But now it seems that we can get rid of it and flatten the test struct again. Let me know which way do you prefer!

BTW, I prepended all the test structs with test, in order to prevent it from being accidentally used in the non-test part of the package. Is this too verbose?

I think flattening things out will be simplest. Also, I don't think the rest of the package can use the test code, at least not if compiled with Bazel (not sure how go build works), so no need to prefix with test.

Ack, removed testFileParams

ola-rozenfeld · 2020-07-07T18:17:24Z

go/pkg/filemetadata/filemetadata_test.go

+	target  string
+}
+
+func (sc *testSymlinkCreationResult) cleanup() {


If everything's under temp, why do you need to clean it up?

This is actually to follow

remote-apis-sdks/go/pkg/filemetadata/filemetadata_test.go

Line 38 in f9b4b0d

defer os.RemoveAll(filename)

Not sure I follow... Do you mean in the next patch? I meant to simplify as instead of cleaning up manually, create everything under a TempDir directory -- pass this directory to everything that creates files or symlinks if you need to reuse it.

Sorry about the confusion. My point is that the existing file tests are doing this clean up manually (even if those filenames are temporary and unique), therefore I followed that for consistency.

Hmm, right, now I wonder why I did that :-) But sure, let's keep it, then.

Oops, i actually removed the Clean() method..

go/pkg/filemetadata/filemetadata.go

ola-rozenfeld · 2020-07-08T19:19:40Z

go/pkg/filemetadata/filemetadata_test.go

+	t.Helper()
+
+	randBytes := make([]byte, 16)
+	rand.Read(randBytes)


Simplifying: pass the symlink name as a parameter, so you don't need to create it and return it. You can base it on the name of the test case, if you don't want to add another variable. That way, you don't need the testSymlinkCreationResult at all, you just return the file name.

ola-rozenfeld · 2020-07-08T20:48:27Z

go/pkg/filemetadata/filemetadata_test.go

+	target  string
+}
+
+func (sc *testSymlinkCreationResult) cleanup() {


Not sure I follow... Do you mean in the next patch? I meant to simplify as instead of cleaning up manually, create everything under a TempDir directory -- pass this directory to everything that creates files or symlinks if you need to reuse it.

k-ye · 2020-07-09T05:57:02Z

Hi Ola,

I just had a team meeting and it seems that supporting only files and directories would cover the major use cases for us. I'm still happy to polish this specific PR and get it merged, if you think it's useful. But otherwise, I'll lower the priority of #146 and come back to this later (maybe we can have a meeting some time to talk about how to support it, if you don't have a plan already..).

ola-rozenfeld · 2020-07-09T14:57:41Z

Glad to hear that, because supporting symlinks properly would be a lot of work. I have a plan in mind, would be good if I wrote it down some time -- if I do, I'll share it with you. Yes, I think this PR is a good start, and we should get it merged. All I had is a few simplifying nits, so should be easy. Thank you for doing this!

…

On Thu, Jul 9, 2020 at 1:57 AM Ye Kuang ***@***.***> wrote: Hi Ola, I just had a team meeting and it seems that supporting only files and directories would cover the major use cases for us. I'm still happy to polish this specific PR and get it merged, if you think it's useful. But otherwise, I'll lower the priority of #146 <#146> and come back to this later (maybe we can have a meeting some time to talk about how to support it, if you don't have a plan already..). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#153 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGFAVOERRFKS3N2KKNZM6JLR2VL3ZANCNFSM4OSRYV7A> .

k-ye · 2020-07-10T05:41:17Z

Glad to hear that, because supporting symlinks properly would be a lot of work. I have a plan in mind, would be good if I wrote it down some time -- if I do, I'll share it with you. Yes, I think this PR is a good start, and we should get it merged. All I had is a few simplifying nits, so should be easy. Thank you for doing this!
…
On Thu, Jul 9, 2020 at 1:57 AM Ye Kuang @.***> wrote: Hi Ola, I just had a team meeting and it seems that supporting only files and directories would cover the major use cases for us. I'm still happy to polish this specific PR and get it merged, if you think it's useful. But otherwise, I'll lower the priority of #146 <#146> and come back to this later (maybe we can have a meeting some time to talk about how to support it, if you don't have a plan already..). — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#153 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGFAVOERRFKS3N2KKNZM6JLR2VL3ZANCNFSM4OSRYV7A .

Thanks! I don't see new comments since last around of review, will wait for more suggestions :)

ola-rozenfeld · 2020-07-13T15:36:28Z

Sorry for disappearing, I was off for a few days!

Add symlink info to file metadata

6d88e63

googlebot added the cla: yes The author signed a CLA label Jul 7, 2020

k-ye added 3 commits July 7, 2020 17:15

use pointer type

911fda4

simplify

85722c7

remove print

8743bc7

ola-rozenfeld reviewed Jul 7, 2020

View reviewed changes

k-ye added 5 commits July 8, 2020 09:51

IsDangling

b9faa1d

fix tests

e3df0d0

fmt

552b0f1

rm unused

97768db

fix msg

f724e1c

k-ye commented Jul 8, 2020

View reviewed changes

go/pkg/filemetadata/filemetadata.go Outdated Show resolved Hide resolved

k-ye requested a review from ola-rozenfeld July 8, 2020 02:33

set target even if dangling

447e40b

ola-rozenfeld reviewed Jul 8, 2020

View reviewed changes

apply fix

9d3ac56

k-ye requested a review from ola-rozenfeld July 9, 2020 00:20

func name

32aa7fc

rm testFileParams

12076a2

ola-rozenfeld approved these changes Jul 14, 2020

View reviewed changes

ola-rozenfeld merged commit ef239a1 into bazelbuild:master Jul 14, 2020

k-ye deleted the sym branch July 16, 2020 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add symlink info to file metadata #153

Add symlink info to file metadata #153

k-ye commented Jul 7, 2020 •

edited

Loading

ola-rozenfeld Jul 7, 2020

k-ye Jul 8, 2020

ola-rozenfeld Jul 7, 2020

k-ye Jul 8, 2020 •

edited

Loading

ola-rozenfeld Jul 13, 2020

k-ye Jul 14, 2020

ola-rozenfeld Jul 7, 2020

k-ye Jul 8, 2020

ola-rozenfeld Jul 8, 2020

k-ye Jul 9, 2020

ola-rozenfeld Jul 13, 2020

k-ye Jul 14, 2020

ola-rozenfeld Jul 8, 2020

k-ye Jul 9, 2020

ola-rozenfeld Jul 8, 2020

k-ye commented Jul 9, 2020

ola-rozenfeld commented Jul 9, 2020 via email

k-ye commented Jul 10, 2020

ola-rozenfeld commented Jul 13, 2020

Add symlink info to file metadata #153

Add symlink info to file metadata #153

Conversation

k-ye commented Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k-ye Jul 8, 2020 • edited Loading

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

k-ye commented Jul 9, 2020

ola-rozenfeld commented Jul 9, 2020 via email

k-ye commented Jul 10, 2020

ola-rozenfeld commented Jul 13, 2020

k-ye commented Jul 7, 2020 •

edited

Loading

k-ye Jul 8, 2020 •

edited

Loading