Enable containersV2 by default
#369
Merged
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
Generate changelog in
|
wip: Make containerV2 default to true wip: Make containerV2 default to true wip: re-add containerv2 flag to test configs. add debug logs remove more containerV2 flags try omitempty use setToDefaults outparamcheck use field.Addr().Interface() pass addr of conf don't take addr of unaddressable value use *bool set default update config_test set containerv2: false for existing configs nil check update test expectation Remove debug logs. Add nl
mpritham
force-pushed
the
pmarupaka/containers-v2-default
branch
from
7161e99 to
4f135ba
Compare
mpritham
commented
Jan 9, 2024
| @@ -144,7 +144,7 @@ func TestMainMethodWithoutCustomConfig(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestMainMethodContainerWithoutCustomConfig(t *testing.T) { | |||
| output := testContainerSupportEnabled(t, "foo", "-XX\\:InitialRAMPercentage=75.0 -XX\\:MaxRAMPercentage=75.0 -XX\\:ActiveProcessorCount=2", []string{}) | |||
| output := testContainerSupportEnabled(t, "foo", "", []string{"-Xmx", "-Xms"}) | |||
There was a problem hiding this comment.
If there is a no custom config, or there is a failure to read the custom config, we use the new default behavior, hence this change.
mpritham
commented
Jan 9, 2024
| @@ -3,3 +3,5 @@ configVersion: 1 | |||
| jvmOpts: | |||
| - '-Xmx1g' | |||
| dangerousDisableContainerSupport: true | |||
| experimental: | |||
There was a problem hiding this comment.
Setting the flag to false to retain tests for old behavior for now.
mpritham
commented
Jan 9, 2024
| @@ -267,6 +267,10 @@ func parseCustomConfig(yamlString []byte) (PrimaryCustomLauncherConfig, error) { | |||
| "subProcess config %s", name) | |||
| } | |||
| } | |||
| if config.Experimental.ContainerV2 == nil { | |||
| var trueVal = true | |||
| config.Experimental.ContainerV2 = &trueVal | |||
There was a problem hiding this comment.
I've seen this pattern elsewhere, but it would be nice if I could make this one line.
There was a problem hiding this comment.
I don't think there's anything in the standard library for it :( but there are some pretty small deps for doing something like this:
package main
import (
"fmt"
"k8s.io/utils/ptr"
)
func main() {
a := ptr.To(false)
fmt.Printf("%T %v", a, *a)
}There was a problem hiding this comment.
I see. I'm leaning towards not adding the dep since we use this in one place in the implementation (and also once in the tests).
andybradshaw
approved these changes
Jan 10, 2024
|
Thanks @mpritham and @andybradshaw! 👍 |
|
Released 1.101.0 |
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.
Before this PR
In #361 we introduced the
containersV2flag. After observing improvements across several stacks, we'd like to enable the functionality guarded by the flag as the default behavior. We'd like to keep the option open for devs to disable the behavior if needed, by setting thecontainersV2flag tofalse.After this PR
==COMMIT_MSG==
Enable
containersV2by default.==COMMIT_MSG==
Possible downsides?
N/A.