Use dependency injection in tasks #3897
Conversation
|
SonarCloud Quality Gate failed. |
|
Also, I noticed that the JibTask interface had a setJibExtension function, but it wasn't implemented by all tasks registered by the jib plugin, so there was a bit of duplication for other tasks. Not sure if that was by design or a bug. Perhaps JibTask was only meant to be implemented by the main 3 tasks? Regardless, JibTask is now empty. |
Sineaggi
force-pushed
the
use-dependency-injection-in-tasks
branch
from
2ba8524
to
8203305
Compare
|
Bump |
Sineaggi
force-pushed
the
use-dependency-injection-in-tasks
branch
from
8203305
to
31997f7
Compare
|
SonarCloud Quality Gate failed. |
|
Will be reviewing this along with #3892 - thanks again for your patience! |
|
@Sineaggi Is there any way we could keep the public I'm mainly concerned this might be a breaking change for users, given the context that the
Hmm yeah, I'm not entirely sure why the skaffold tasks didn’t implement the |
We could keep the public setter for backwards compatibility. Perhaps we could require the extension to be non-null in the setter, but still have it initialized via the constructor? |
There was a problem hiding this comment.
We could keep the public setter for backwards compatibility.
Perhaps we could require the extension to be non-null in the setter, but still have it initialized via the constructor?
I think that would be a good idea. Could we update this implementation so that the extension is required to be non-null in the private field and getter, gets initialized via the constructor, but has the public setter available? Thanks!
|
|
||
| @Override | ||
| public BuildImageTask setJibExtension(JibExtension jibExtension) { | ||
| this.jibExtension = jibExtension; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Let's keep this public method. I think this was orginally made public to allow BuildImageTask to be further customized: #612 (comment). Removing the method may be a breaking change for folks who are going this route.
| * @param jibExtension the {@link JibExtension} | ||
| * @return this | ||
| */ | ||
| Task setJibExtension(JibExtension jibExtension); |
There was a problem hiding this comment.
As mentioned in the previous comment, let's keep this method to maintain backwards compatibility.
Sineaggi
force-pushed
the
use-dependency-injection-in-tasks
branch
from
31997f7
to
7276c95
Compare
Sineaggi
force-pushed
the
use-dependency-injection-in-tasks
branch
from
bab01b5
to
f325370
Compare
Replaces setters with constructor injection. Allows us to remove nullable annotation, make the private JibExtension fields final, and remove null checks.
Unfortunately there's no
TaskContainer#registeroverload that takes a name, type, constructor args and a configuration action, we work around that by usingTaskContainer#registerfirst then callingTaskProvider#configure