-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/configuration as code compliance #56
Feature/configuration as code compliance #56
Conversation
@oleg-nenashev why is this PR not being built? |
This is a pull request not complete at all. It can be marked as DO NOT MERGE but it seems I don't have the permission to do that. Also it is not built and I don't know the triggering strategy. TODO : discover the strategy of triggering. |
Maybe it was not built due to the infra glitch. I do not see specific
reasons in the code
…On Sat, Jan 12, 2019, 00:38 affe ***@***.*** wrote:
This is a pull request not complete at all. It can be marked as DO NOT
MERGE but it seems I don't have the permission to do that. Also it is not
built and I don't know the triggering strategy. TODO : discover the
strategy of triggering.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#56 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AC3IoFssTXiKblg4-IE31F8DofHs_uPgks5vCSBagaJpZM4Z8I1->
.
|
Really odd why this PR is not being built. I've created a new dummy PR just to test if this feature is still working, and it does. I have a new build being triggered for #58 -> https://ci.jenkins.io/job/Plugins/job/external-workspace-manager-plugin/view/change-requests/job/PR-58/ @imaffe Could it be a cause that maybe you've started this branch on top of Martin's branch, containing some of his commits? |
…the .yaml file with Martin's
f6ccfea
to
5474c94
Compare
Currently the tests can run and is fixable by change the .yaml file I suppose. However there is another error log from |
The current build can complete except for some test failures that I haven't get to yet. It seems the cause for "error exit" is the config .yaml file didn't include "jenkins" entry. This will cause an interrupt. Not sure if it is an issue ? I'll try to read the implementation of JCasC since the configuration in exws requires cascading configuration ( that is diskPools and its components must be configured and is no documented in the developer's guide). This might take a while since JCasC is a big plugin as it seems :) |
@imaffe Symbols should not be needed for Pipeline steps, because you already have |
@oleg-nenashev Thanks for the help.I'll try to understand the underlying mechanism of how the CasC work. It seems JCasC has multiple ways to get the name of the component it wants to configure. Anyway it might take me some time to understand what's really going on inside there. |
The JCasC source code is too heavy for me now. Maybe understanding what getFunctionName() and @symbol really does is more helpful. |
src/main/java/org/jenkinsci/plugins/ewm/steps/ExwsAllocateStep.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/ewm/steps/ExwsAllocateStep.java
Outdated
Show resolved
Hide resolved
@oleg-nenashev @martinda I'm planning to write my first unit test code in Java (yes, first time in java LOL) for the JCasC test. The draft idea is :
And this will lead to following changes in code, since I don't know if they match the coding convention, i have some questions :
And hope you can give me some advice on some new test points or useful techniques in this. Thank you so much ! |
@imaffe When you add a dependency which is only used for testing, you need to specify the est scope As for adding a test dependency on |
I had a look around and snakeyaml was the best option I saw. There's a TODO here for providing a library to make testing exported yaml easy, currently not done though: |
@timja Is there anyone already on this now ? If not i might offer some help like writing some documentation and give some examples. Also it would be great if you can give me a link to the related issue if there is one. Thanks so much ! |
I've created a github issue: |
src/test/java/org/jenkinsci/plugins/ewm/casc/ConfigAsCodeTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/jenkinsci/plugins/ewm/casc/ConfigAsCodeTest.java
Outdated
Show resolved
Hide resolved
Oleg and Tim says we won't need this. But I didn't found related documentations either. and without @symbol it worked fine. Interesting |
the symbol by default is based off the descriptor class name, with or without you've mapped it to the same value, (pretty sure, may be wrong though...) |
I think you've accidentally committed some kind of swap file: |
377351f
to
5739244
Compare
I removed the accidental swap file and added the pattern to my |
I'd like to have this going through. Is there anything else that should be done? Can each of the reviewers please mark the discussions as Resolved, if that's the case? Or maybe add additional comments if needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what that file is about.
@@ -0,0 +1,16 @@ | |||
jenkins: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this file needed at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it can be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need help with the code I introduced. Please have a look:
- diskRefId: "master-node-disk" | ||
nodeMountPoint: "/tmp/master-node" | ||
|
||
# TODO [Martin]: The following code crashes JCasC for reasons I do not understand |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyone knows what is wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be that the expected singleton is not a singleton?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get this error when I comment out that code:
[ERROR] Tests run: 2, Failures: 1, Errors: 1, Skipped: 0, Time elapsed: 5.689 s <<< FAILURE! - in org.jenkinsci.plugins.ewm.casc.ConfigAsCodeTest
[ERROR] shouldSupportConfigurationAsCode(org.jenkinsci.plugins.ewm.casc.ConfigAsCodeTest) Time elapsed: 0.047 s <<< ERROR!
java.lang.NullPointerException
at io.jenkins.plugins.casc.model.Mapping.lambda$clone$0(Mapping.java:71)
at java.util.HashMap$EntrySpliterator.forEachRemaining(HashMap.java:1691)
at java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:580)
at io.jenkins.plugins.casc.model.Mapping.clone(Mapping.java:71)
at io.jenkins.plugins.casc.model.Mapping.clone(Mapping.java:11)
at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1374)
at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151)
at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174)
at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418)
at io.jenkins.plugins.casc.model.Sequence.clone(Sequence.java:43)
at io.jenkins.plugins.casc.model.Sequence.clone(Sequence.java:9)
at io.jenkins.plugins.casc.model.Mapping.lambda$clone$0(Mapping.java:71)
at java.util.HashMap$EntrySpliterator.forEachRemaining(HashMap.java:1691)
at java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:580)
at io.jenkins.plugins.casc.model.Mapping.clone(Mapping.java:71)
at io.jenkins.plugins.casc.model.Mapping.clone(Mapping.java:11)
at io.jenkins.plugins.casc.model.Mapping.lambda$clone$0(Mapping.java:71)
at java.util.HashMap$EntrySpliterator.forEachRemaining(HashMap.java:1691)
at java.util.stream.ReferencePipeline$Head.forEach(ReferencePipeline.java:580)
at io.jenkins.plugins.casc.model.Mapping.clone(Mapping.java:71)
at io.jenkins.plugins.casc.ConfigurationAsCode.configureWith(ConfigurationAsCode.java:641)
at io.jenkins.plugins.casc.ConfigurationAsCode.configureWith(ConfigurationAsCode.java:545)
at io.jenkins.plugins.casc.ConfigurationAsCode.configure(ConfigurationAsCode.java:520)
at io.jenkins.plugins.casc.ConfigurationAsCode.configure(ConfigurationAsCode.java:503)
at org.jenkinsci.plugins.ewm.casc.ConfigAsCodeTest.shouldSupportConfigurationAsCode(ConfigAsCodeTest.java:43)
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:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:548)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.lang.Thread.run(Thread.java:748)
[ERROR] exportConfiguration(org.jenkinsci.plugins.ewm.casc.ConfigAsCodeTest) Time elapsed: 0.272 s <<< FAILURE!
java.lang.AssertionError: expected:<{exwsGlobalConfigurationDiskPools={diskPools=[{diskPoolId=diskpool1, disks=[{diskId=disk1, displayName=disk one display name, masterMountPoint=/tmp}], displayName=diskpool1 display name}]}, exwsGlobalConfigurationTemplates={templates=[{label=all, nodeDiskPools=[{diskPoolRefId=dp1, nodeDisks=[{diskRefId=dp1refid1, nodeMountPoint=/tmp/template11}, {diskRefId=dp1refid2, nodeMountPoint=/tmp/template12}]}, {diskPoolRefId=dp2, nodeDisks=[{diskRefId=dp2refid1, nodeMountPoint=/tmp/template21}]}]}]}}> but was:<null>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:834)
at org.junit.Assert.assertEquals(Assert.java:118)
at org.junit.Assert.assertEquals(Assert.java:144)
at org.jenkinsci.plugins.ewm.casc.ConfigAsCodeTest.exportConfiguration(ConfigAsCodeTest.java:93)
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:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:548)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:298)
at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:292)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.lang.Thread.run(Thread.java:748)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would take a while for me to look at these stuffs. And my local repository cannot run local tests due to some unexpected error. (And that's why i didn't follow this PR after that break down ..... LOL )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be enough if you define the node as:
nodes:
- dumb:
mode: NORMAL
name: "agent1"
However I'm still trying to figure out how to define the nodeProperties
😕
numExecutors: 5 | ||
scmCheckoutRetryCount: 2 | ||
mode: NORMAL | ||
nodeProperties: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I read the nodeProperties
map in the unit test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below for my attempt at reading nodeProperties
.
@@ -42,11 +44,17 @@ public void shouldSupportConfigurationAsCode() throws Exception { | |||
String config = resource.toString(); | |||
ConfigurationAsCode.get().configure(config); | |||
|
|||
ExternalWorkspaceProperty.DescriptorImpl exwsNodeProperty = ExtensionList.lookupSingleton(ExternalWorkspaceProperty.DescriptorImpl.class); | |||
List<NodeDiskPool> nodeDiskPools = exwsNodeProperty.getNodeDiskPools(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above code does not work because the descriptor does not have a list of NodeDiskPools
:
Lines 35 to 44 in 08073fc
@Extension | |
public static class DescriptorImpl extends NodePropertyDescriptor { | |
@Nonnull | |
@Override | |
public String getDisplayName() { | |
return Messages.nodes_ExternalWorkspaceProperty_DisplayName(); | |
} | |
} | |
} |
I don't really understand how to fix it... descriptors are still a mystery to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case we need to do some refactoring by moving the List<NodeDiskPool> nodeDiskPools
from ExternalWorkspaceProperty
class to the DescriptorImpl
. I'm not very sure what will break by doing this 🤔
I'll try to look these days over the required changes to be able to do such refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you can change the ExternalWorkspaceProperty
like following, but I'm not sure if this is backwards compatible with previous plugin versions ❗️
public class ExternalWorkspaceProperty extends NodeProperty<Node> {
@DataBoundConstructor
public ExternalWorkspaceProperty(List<NodeDiskPool> nodeDiskPools) {
this.getDescriptor().setDiskPools(fixNull(nodeDiskPools));
}
@Override
public DescriptorImpl getDescriptor() {
return (DescriptorImpl) super.getDescriptor();
}
@Nonnull
public List<NodeDiskPool> getNodeDiskPools() {
return getDescriptor().getNodeDiskPools();
}
@Extension
@Symbol("exwsNodeConfigurationDiskPools")
public static class DescriptorImpl extends NodePropertyDescriptor {
private List<NodeDiskPool> nodeDiskPools = Collections.emptyList();
@Nonnull
@Override
public String getDisplayName() {
return Messages.nodes_ExternalWorkspaceProperty_DisplayName();
}
@Nonnull
public List<NodeDiskPool> getNodeDiskPools() {
return Collections.unmodifiableList(nodeDiskPools);
}
@DataBoundSetter
public void setDiskPools(List<NodeDiskPool> nodeDiskPools) {
this.nodeDiskPools = nodeDiskPools;
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works for the test, but breaks backward compatibility. The ConfigMigrationTest fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking the NodeDiskPool class also needs to change. Descriptors are so mysterious to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like NodeDisk also has to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to change NodeDiskPool
and NodeDisk
? I think it should work even without changing those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I was trying to get the test to pass, and it was failing instead, that's how these recent changes started. Maybe the test needs to be written differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, I think we might not read ExternalWorkspaceProperty like this. There should be a class containing all nodeProperties, and we suppose that class is NodeProperties. Then we do something like NodeProperties.getDescriptor().getProperties(ExternalWorkspaceProperty.class)
.(The syntax is wrong, but should be very same, I'm working on finding that class)
Refactor NodePropertyDescriptor extensions to support JCasC but this breaks backward compatibility with past releases.
|
||
@Test | ||
public void shouldSupportConfigurationAsCode() throws Exception { | ||
URL resource = ConfigAsCodeTest.class.getResource("configuration-as-code.yaml"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you add the tests dependency you can simplify this code:
<dependency>
<groupId>io.jenkins</groupId>
<artifactId>configuration-as-code</artifactId>
<version>1.8</version>
<classifier>tests</classifier>
<scope>test</scope>
</dependency>
then you annotate the test method with:
@ConfiguredWithCode("configuration-as-code.yaml")
and change the rule to be:
@Rule
public JenkinsConfiguredWithCodeRule j = new JenkinsConfiguredWithCodeRule();
All the configuration as code setup bit will be done for you automatically
i.e.
https://github.com/jenkinsci/azure-keyvault-plugin/pull/15/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. First I want to debug what's wrong with the Descriptor. I prefer to work on one problem at a time. See the commit 9d342da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say, we should not refactor the discriptor. According to my obeservation, only those descriptor with a configure
method will need fields in the descriptor, and all those descriptors are global configurations. Node properties is a little bit special, but I take it as a not-top-level describable, as there were no configure()
method defined previously. I also guess there would be a describable serve as the top-level who appear in the web UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinda Hi Martin, our previous direction was correct. I all push a new version tomorrow : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @imaffe looking forward to it.
Today, without changing the order of the lists in the yaml file, the list elements show up in a different order, so the test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am stuck.
ExwsStep.DescriptorImpl globalTemplateDescriptor = ExtensionList.lookupSingleton(ExwsStep.DescriptorImpl.class); | ||
List<Template> templates = globalTemplateDescriptor.getTemplates(); | ||
assertThat(templates.get(0).getLabel(), is("all")); | ||
System.out.println("Should be dp1: "+templates.get(0).getNodeDiskPools().get(0).getDiskPoolRefId()); | ||
System.out.println("Should be dp2: "+templates.get(0).getNodeDiskPools().get(1).getDiskPoolRefId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am printing the two disk pools, and they both have the same refId value. This is not normal. Something is wrong with the descriptor(s), but I do not know what it is. It's like all instances of the list are the same. The above prints:
Should be dp1: dp2
Should be dp2: dp2
Sorry, I have completely dropped the ball on this. I have pushed a fix for the get(0) problem, but the test still fails in the same way. It's as if there is only one instance of the Descriptor and the whole list gets the same value for all elements. |
@alexsomai Hi Alex, I got some questions, in |
@imaffe Hi Yufei, I think this was rather a design flaw. I don't remember the reason for defining
I'm open to other suggestions. :) |
This is replaced by #68 . Closing. |
Related to PR#51
Summary of this pull request:
CC