feat (jkube-kit) : Add support for specifying initContainers via resource configuration (#1316) #1969
Conversation
This file contains 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
Eclipse JKube CI ReportStarted new GH workflow run for #1969 (2023-02-08T12:48:12Z) ⚙️ JKube E2E Tests (4124097593)
|
Codecov Report
@@ Coverage Diff @@
## master #1969 +/- ##
============================================
+ Coverage 54.20% 55.54% +1.34%
- Complexity 4061 4263 +202
============================================
Files 480 481 +1
Lines 21067 21158 +91
Branches 2810 2834 +24
============================================
+ Hits 11420 11753 +333
+ Misses 8483 8202 -281
- Partials 1164 1203 +39
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
rohanKanojia
force-pushed
the
pr/issue1316
branch
18 times, most recently
from
49e3b26
to
adfda2a
Compare
|
It's a bit strange. Sonar is reporting less than ~50% code coverage for ControllerResourceConfig, InitContainerConfig, MappingConfig, Arguments. But my IDE shows full test coverage 😕 Also, Sonar's code coverage (68.4%) does not seem to be similar to codecov (87.1%) |
rohanKanojia
force-pushed
the
pr/issue1316
branch
from
adfda2a
to
c15a4c8
Compare
rohanKanojia
force-pushed
the
pr/issue1316
branch
from
c15a4c8
to
c10a2be
Compare
|
needs conflict resolution |
rohanKanojia
force-pushed
the
pr/issue1316
branch
from
c10a2be
to
de561b9
Compare
…urce configuration (eclipse-jkube#1316) + Deprecate all controller specific configuration options in ResourceConfig. Move all controller specific options to nested controller configuration. + Add controller(s) configuration option in ResourceConfig + Add initContainres configuraiton option in Controller configuration Signed-off-by: Rohan Kumar <rohaan@redhat.com>
…er configurtion Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/issue1316
branch
from
de561b9
to
bce34ee
Compare
manusa
reviewed
Feb 7, 2023
Comment on lines
123
to
136
| public ControllerResourceConfig getController() { | ||
| if (controller == null) { | ||
| controller = createNewControllerConfig(); | ||
| } | ||
| return controller; | ||
| } | ||
|
|
||
| public List<ControllerResourceConfig> getControllers() { | ||
| if (controllers == null || controllers.isEmpty()) { | ||
| controllers = new ArrayList<>(); | ||
| controllers.add(createNewControllerConfig()); | ||
| } | ||
| return controllers; | ||
| } |
There was a problem hiding this comment.
Still reviewing the rest of the code
Not sure how we're dealing with the case where a user specifies both controller and controllers. Or the case where the user is specifying all of the other options.
IMHO getController should remain private and access to controller information should only be allowed through the list (I think we should be doing something similar elsewhere).
Following are the cases I can gather:
- No controller, no controllers, no top-level controller settings:
We might want to init a dummy Controller object to avoid NPE, but this is not very elegant (getControllersreturns a single empty element) - No controller, no controllers, but top-level controller settings added:
getControllersreturns the result ofcreateNewControllerConfig - Controller, no controllers, no top-level controller settings:
getControllersreturns a single element with the provided controller - No controller, controllers, no top-level controller settings
getControllersreturns the list of controllers - Controller, controllers, no top-level controller settings
getControllersreturns a merged list of controllers prepending the single controller - Controller or controllers, and top-level controller settings
getControllersreturns a merged list of controllers prepending the single.
top-level settings are ignored since they are deprecated (we might want to warn the user)
There was a problem hiding this comment.
After discussion
Remove the getControllers method and just keep a single getController that's in charge of retrieving or merging the legacy settings. i.e. we don't support multiple controllers.
If both (controller field and legacy fields are present), throw an exception with a helpful message for users to be able to safely migrate to the new config.
In case user is using legacy fields we log a debug message.
manusa
reviewed
Feb 7, 2023
Comment on lines
160
to
165
| protected ControllerResourceConfig getControllerResourceConfig() { | ||
| ResourceConfig resourceConfig = getConfiguration().getResource(); | ||
| if (resourceConfig != null && resourceConfig.getController() != null) { | ||
| return resourceConfig.getController(); | ||
| } | ||
| return ControllerResourceConfig.builder().build(); |
There was a problem hiding this comment.
Related to the previous comment, this method should call getControllers
If there is more than one controller, then we should warn the user.
…ntroller configuration Signed-off-by: Rohan Kumar <rohaan@redhat.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
|
Follow up by documenting the new configuration fields and deprecating the legacy ones (Asciidoc) |
manusa
approved these changes
Feb 8, 2023
|
SonarCloud Quality Gate failed. |
manusa
added a commit
to manusa/jkube
that referenced
this pull request
May 15, 2023
Signed-off-by: Marc Nuri <marc@marcnuri.com>
manusa
added a commit
that referenced
this pull request
May 17, 2023
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Fix #1316
org.eclipse.jkube.kit.commonpackageResourceConfig. Move all controller specific options to nested controller configuration.
Documentation in #1970
Signed-off-by: Rohan Kumar rohaan@redhat.com
Type of change
test, version modification, documentation, etc.)
Checklist