fix(host-cleanup, docker): protect "tempID:latest" docker tag from parallel "werf host cleanup" process - #7049
Merged
alexey-igrychev merged 1 commit intoSep 2, 2025
Conversation
…erf host cleanup" process Signed-off-by: Alexandr Zaytsev <alexandr.zaytsev@flant.com>
nervgh
marked this pull request as draft
August 22, 2025 13:45
nervgh
marked this pull request as ready for review
August 22, 2025 15:18
iapershin
suggested changes
Aug 26, 2025
Comment on lines
+28
to
+30
| type DockerServerBackend struct { | ||
| locker lockgate.Locker | ||
| } |
Contributor
There was a problem hiding this comment.
- I don't see any reason why locker is a part of DockerServerBackend. As I see from the code it could be called independently in a single implementation of DockerServerBackend method. You can get host locker from anywhere in the code by werf.HostLocker().Locker(), so I don't think it should be a part of initialization of docker backend.
- By the way, lockgate.Locker is a very weak abstraction. Ideally, you should declare locker interface on the consumer's side instead importing.
Contributor
Author
There was a problem hiding this comment.
I pass the locker as the argument to constructor for these reasons:
- Prefer explicit over implicit. If we want to use
werf.HostLocker().Locker()we are required to call werf.Init() before. In this case it is implicit dependency. - Simplification of testing. If we have the locker as implicit dependency then assertion of
locker.Acquire()is not so easy as mocking using standard approach.
Contributor
There was a problem hiding this comment.
- It's not about explicit over implicit, but dependency injection.
Ok, left as is, I don't see better approach for this for now - It's interface segregation principle - "Clients should not be forced to depend on methods they do not use".
It makes the code more coupled and rigid. I don't understand why it's not easy for mocking?
Your mock object implements all the methods and your interface requires only one, so it satisfies the interface.
I'm not blocking the PR but encourage you to review this to make code flexible, easily extensible and minimizing dependencies between modules.
iapershin
approved these changes
Aug 27, 2025
Comment on lines
+28
to
+30
| type DockerServerBackend struct { | ||
| locker lockgate.Locker | ||
| } |
Contributor
There was a problem hiding this comment.
- It's not about explicit over implicit, but dependency injection.
Ok, left as is, I don't see better approach for this for now - It's interface segregation principle - "Clients should not be forced to depend on methods they do not use".
It makes the code more coupled and rigid. I don't understand why it's not easy for mocking?
Your mock object implements all the methods and your interface requires only one, so it satisfies the interface.
I'm not blocking the PR but encourage you to review this to make code flexible, easily extensible and minimizing dependencies between modules.
alexey-igrychev
changed the title
alexey-igrychev
approved these changes
Sep 2, 2025
alexey-igrychev
changed the title
alexey-igrychev
merged commit Sep 2, 2025
6fe15ab
into
fix/host-cleanup/respect-host-locking-for-dry-run
13 checks passed
alexey-igrychev
deleted the
fix/build/protect-tempid-tag-from-host-cleanup
branch
September 2, 2025 09:27
nervgh
added a commit
that referenced
this pull request
Sep 2, 2025
…rallel "werf host cleanup" process (#7049) Signed-off-by: Alexandr Zaytsev <alexandr.zaytsev@flant.com>
alexey-igrychev
pushed a commit
that referenced
this pull request
Sep 2, 2025
…rallel "werf host cleanup" process (#7049) Signed-off-by: Alexandr Zaytsev <alexandr.zaytsev@flant.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.