Skip actions for read-only repositories #7477
Conversation
idanovo
added
the
include-changelog
E2E Test Results - DynamoDB Local - Local Block Adapter |
There was a problem hiding this comment.
I don't understand this PR, and I believe @treeverse/product involvement is required. I can understand why I would want some actions not to run, for instance pre-merge validations. But I am not sure why I would want to prevent all actions from running. For instance, it does make sense to run export hooks (and we could make it easier by exposing e.g. the region to them).
So I believe we need to discuss this, maybe with a design partner.
There was a problem hiding this comment.
Thanks!
Given that it's an explicit requirement in the product definition, obviously my previous objections are moot. The code itself is good, with a comment about the use of deepcopy -- I would much prefer if you could address it.
It is unfortunate that the hooks API forces us to repeat the same code in so many places! I worry that we will miss some change in one hook call. But this PR is about readonly repos more than it is about the hooks API, so I don't think we should rewrite that API just for safety here.
pkg/graveler/graveler_test.go
Outdated
| @@ -1737,12 +1748,15 @@ func TestGraveler_PreCommitHook(t *testing.T) { | |||
| if tt.hook { | |||
| g.SetHooksHandler(h) | |||
| } | |||
| repo := deepcopy.Copy(repository).(*graveler.RepositoryRecord) | |||
There was a problem hiding this comment.
This is scary: AFAICU deepcopy.Copy can only copy exported (uppercased) fields. It so happens that RepositoryRecord and Repository are both plain-data objects -- but there is no guarantee that this will be true.
I believe it would be safer and easier to understand if we just used a separate object repositoryRO or something. I think it would also be safer.
Part of #7465