JENKINS-46253 Add support for subPath to all volume types #1031

hameno · 2021-09-15T11:33:00Z

Includes work from Miguel Campos (#1024)

refs JENKINS-46253

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

Includes work from Miguel Campos (jenkinsci#1024) refs JENKINS-46253

mcamposv · 2021-09-21T17:26:57Z

Dear Hameno,
I'm no expert however before I placed my pull request I was told that we should not modify the existing API methods in a class.
if you check my initial commit I changed the
sprylab@a309ac1#diff-76b7e7535ea9463d5389851a958cc07955f2fcfbe99bb393079fdc6f70b0e46f

I created a new constructor but I maintained the old one that calls the new one:
public ConfigMapVolume(String mountPath, String configMapName, Boolean optional) {
this(mountPath, configMapName, optional, (String) null);
}

If I'm not seeing wrong you changed the constructor but did not maintain the old one for compatibility purpose that might be a reason for the pull request to be rejected.
could you please amend your code to maintain compatibility?

Thanks.

Vlatombe

Please revert any hunk unrelated to the change you want to accomplish.

Vlatombe · 2021-10-04T06:33:58Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java

 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import java.nio.file.Paths;
+import java.util.*;


Don't use wildcard imports in production code. Please revert.

Vlatombe · 2021-10-04T06:34:03Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java

-import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.combine;
-import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.isNullOrEmpty;
-import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.substituteEnv;
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.*;


Ditto above.

Vlatombe · 2021-10-04T06:34:23Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodVolumes.java

+import java.util.List;
+


Please revert

Vlatombe · 2021-10-04T06:36:15Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/volumes/ConfigMapVolume.java

+import org.jenkinsci.Symbol;
+import org.kohsuke.stapler.DataBoundConstructor;


Please avoid this kind of change that is just making reviews and blaming more complex.

Vlatombe · 2021-10-04T06:39:08Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java

+                    // We need to normalize the subPath, or we can end up in really hard to debug issues Just in case.
+                    final String subPath = substituteEnv(Paths.get(volumeSubPath).normalize().toString().replace("\\", "/"));


Is that normalization really needed? What happens if the agent is on Windows?

Hello Viatombe,
I'm answering this because this piece of code is also in my original PR wich was intended only for ConfigMap volumes.
I think the answer applies on both.
To be honest I do not know what will happen in a windows container.
But what I can see is that that normalization is there since 4 years ago. in commit: 43afca4c3209dc6b1d8b768b157ab53cc12da052 made by Alex Johnson ajohnson@bombora.com
The only modification I did was to surround it with an if to control whether the new parameter is empty or not.
So I guess that piece of code is as secure or insecure as it was before and is out of the intention of this PR to modify/evaluate that.

It's fine, I didn't see that the same code was applied to mountPath.

Vlatombe · 2021-10-04T06:48:38Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/volumes/ConfigMapVolume.java

+     @DataBoundConstructor
+    public ConfigMapVolume(String mountPath, String configMapName, Boolean optional, String subPath) {


Instead of this, you could add a setter for subPath annotated with @DataBoundSetter

Hello, viatombe,
I'm going to need a bit more explanation on why this is not the correct way of doing things. or how you preffer me to do it
I do not know why is wrong the go from 3 parameters in the constructor to 4.
I do not know the difference between @DataBoundConstructor and @DataboundSetter.
however @DataBoundSetter was not used anywhere in the previous code.
and I'm just adding a new parameter to the constructor. keeping the old one for compatibility issues.
I do not know where is the issue. please explain.
I'm very happy to learn the right way of doing things.

You can read the explanation here -- Constructor vs. setters.
In that case ideally only mountPath and configMapName should be left in the constructor since they are mandatory, and optional and subPath should be set using setters.

Thanks Vincent,
the link was quite useful and I understand the difference.
I've done the modifications on my original code/PR
All the other issues are not in my original code/PR.
Please consider merging PR while @hameno fixes the issues on his side.

jgournet · 2021-11-17T21:56:35Z

@hameno : would you be able to progress this further after the comments ? This is a great feature that is direly missing

hameno · 2021-11-17T22:00:08Z

@hameno : would you be able to progress this further after the comments ? This is a great feature that is direly missing

Unfortunately I won't have capacity until next year. I found a workaround which works for us so this currently doesn't have priority

jgournet · 2021-11-17T22:07:33Z

May I ask what workaround you're using ? (right now, I'm just mounting the whole volume and hardcoding paths ... that will work, but if there's better, I wouldn't mind ;) )

hameno · 2021-11-17T22:11:44Z

(On mobile right now, so not exactly sure how it's called)
I'm using the agent yaml template to adjust the Pod spec

EricDales · 2022-05-25T10:31:23Z

Hello, any chance this pull request will be finalized ?

hameno force-pushed the feature/subpath branch from 92b7695 to a6928ef Compare September 15, 2021 11:54

Add support for subPath to all volume types

92fc68c

Includes work from Miguel Campos (jenkinsci#1024) refs JENKINS-46253

hameno force-pushed the feature/subpath branch from a6928ef to 92fc68c Compare September 15, 2021 12:21

Vlatombe reviewed Oct 4, 2021

View reviewed changes

Vlatombe added the enhancement Improvements label Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-46253 Add support for subPath to all volume types #1031

JENKINS-46253 Add support for subPath to all volume types #1031

hameno commented Sep 15, 2021

mcamposv commented Sep 21, 2021

Vlatombe left a comment

Vlatombe Oct 4, 2021

Vlatombe Oct 4, 2021

Vlatombe Oct 4, 2021

Vlatombe Oct 4, 2021

Vlatombe Oct 4, 2021

mcamposv Oct 4, 2021

Vlatombe Oct 4, 2021

Vlatombe Oct 4, 2021

mcamposv Oct 4, 2021

Vlatombe Oct 4, 2021 •

edited

Loading

mcamposv Oct 4, 2021

jgournet commented Nov 17, 2021

hameno commented Nov 17, 2021

jgournet commented Nov 17, 2021

hameno commented Nov 17, 2021

EricDales commented May 25, 2022

		import org.jenkinsci.Symbol;
		import org.kohsuke.stapler.DataBoundConstructor;

		// We need to normalize the subPath, or we can end up in really hard to debug issues Just in case.
		final String subPath = substituteEnv(Paths.get(volumeSubPath).normalize().toString().replace("\\", "/"));

		@DataBoundConstructor
		public ConfigMapVolume(String mountPath, String configMapName, Boolean optional, String subPath) {

JENKINS-46253 Add support for subPath to all volume types #1031

Are you sure you want to change the base?

JENKINS-46253 Add support for subPath to all volume types #1031

Conversation

hameno commented Sep 15, 2021

mcamposv commented Sep 21, 2021

Vlatombe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vlatombe Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgournet commented Nov 17, 2021

hameno commented Nov 17, 2021

jgournet commented Nov 17, 2021

hameno commented Nov 17, 2021

EricDales commented May 25, 2022

Vlatombe Oct 4, 2021 •

edited

Loading