Allow configuring max aliases for a collection #1375
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
Codecov Report
@@ Coverage Diff @@
## master #1375 +/- ##
============================================
- Coverage 79.61% 79.52% -0.10%
- Complexity 804 806 +2
============================================
Files 66 66
Lines 2330 2344 +14
Branches 325 326 +1
============================================
+ Hits 1855 1864 +9
- Misses 375 380 +5
Partials 100 100
|
timja
approved these changes
May 8, 2020
…ases config This needs to be a property/env since the context is not yet configurable as it has yet to read any YAML and it might be blocked due to alias limit. Adding the reading of the property/env to ConfigurationContext makes sense to avoid YamlUtils to constantly reading the property on every file read when it attempts to merge all the sources.
jetersen
commented
May 12, 2020
Comment on lines
+42
to
+46
| String prop = Util.fixEmptyAndTrim(System.getProperty( | ||
| CASC_YAML_MAX_ALIASES_PROPERTY, | ||
| System.getenv(CASC_YAML_MAX_ALIASES_ENV) | ||
| )); | ||
| yamlMaxAliasesForCollections = NumberUtils.toInt(prop, 50); |
There was a problem hiding this comment.
@timja Please see 57aebdd commit message for my reasoning to use a property.
This needs to be a property/env since the context is not yet configurable as it has yet to read any YAML
and it might be blocked due to alias limit.Adding the reading of the property/env to ConfigurationContext
makes sense to avoid YamlUtils to constantly reading the property on every file read
when it attempts to merge all the sources.
|
I would like to add tests for aliases and demo. |
timja
approved these changes
May 12, 2020
jetersen
changed the title
6 tasks
|
No need for documentation it as the exception will tell them what to do. |
timja
reviewed
May 24, 2020
| @@ -27,14 +26,14 @@ | |||
| @Test | |||
| public void testAMaxOfOneEnv() { | |||
| env.set(ConfigurationContext.CASC_YAML_MAX_ALIASES_ENV, "1"); | |||
| assertThrows(YAMLException.class, () -> | |||
| assertThrows(ConfiguratorException.class, () -> | |||
There was a problem hiding this comment.
assertThrows does not allow for asserting the exception message.
There was a problem hiding this comment.
It returns the exception and then you can assert on it
There was a problem hiding this comment.
Oh it returns the exception so sure 😅
There was a problem hiding this comment.
The stack trace somewhat hides it but it is still visible.
io.jenkins.plugins.casc.ConfiguratorException: Failed to read YamlSource: file:/C:/git/code/configuration.as.code.plugin/test-harness/target/test-classes/io/jenkins/plugins/casc/maxAliasesLimit.yml
at io.jenkins.plugins.casc.yaml.YamlUtils.merge(YamlUtils.java:48)
at io.jenkins.plugins.casc.yaml.YamlUtils.loadFrom(YamlUtils.java:127)
at io.jenkins.plugins.casc.ConfigurationAsCode.configureWith(ConfigurationAsCode.java:618)
at io.jenkins.plugins.casc.ConfigurationAsCode.configure(ConfigurationAsCode.java:587)
at io.jenkins.plugins.casc.ConfigurationAsCode.configure(ConfigurationAsCode.java:576)
at io.jenkins.plugins.casc.YamlMaxAliasesCollection.lambda$testAMaxOfOneProp$1(YamlMaxAliasesCollection.java:42)
at org.junit.Assert.assertThrows(Assert.java:1001)
at org.junit.Assert.assertThrows(Assert.java:981)
at io.jenkins.plugins.casc.YamlMaxAliasesCollection.testAMaxOfOneProp(YamlMaxAliasesCollection.java:40)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:597)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:288)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:282)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.lang.Thread.run(Thread.java:748)
Caused by: io.jenkins.plugins.casc.ConfiguratorException: Number of aliases for non-scalar nodes exceeds the specified max=1
You can increase the maximum by setting an environment variable or property
ENV: CASC_YAML_MAX_ALIASES="100"
PROPERTY: -Dcasc.yaml.max.aliases="100"
at io.jenkins.plugins.casc.yaml.YamlUtils.read(YamlUtils.java:66)
at io.jenkins.plugins.casc.yaml.YamlUtils.merge(YamlUtils.java:38)
... 21 more
timja
approved these changes
May 24, 2020
timja
changed the title
timja
changed the title
|
jacoco does not like assertThrows for code coverage 😓 |
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.
fixes #1374
Your checklist for this pull request
🚨 Please review the guidelines for contributing to this repository.