-
Notifications
You must be signed in to change notification settings - Fork 89
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
Check volume requirement on all containers. #470
Check volume requirement on all containers. #470
Conversation
@@ -250,10 +251,7 @@ PodSpec createPodSpec(AppDeploymentRequest appDeploymentRequest) { | |||
podSpec.withTolerations(this.deploymentPropertiesResolver.getTolerations(deploymentProperties)); | |||
|
|||
// only add volumes with corresponding volume mounts |
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 comment needs to be moved to the new location of where we add volumes to volume mounts.
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.
moved
public void deployWithVolumesAndVolumeMountsOnAdditionalContainer() throws Exception { | ||
AppDefinition definition = new AppDefinition("app-test", null); | ||
Map<String, String> props = new HashMap<>(); | ||
props.put("spring.cloud.deployer.kubernetes.volumes", "[{name: 'config', configMap: {name: promtail-config, items: [{key: promtail.yaml, path: promtail.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.
Nitpick: Could we add more volumes so that we can make sure the correct one is returned?
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.
done
One more request can we have a negative test? 🙏 |
Added negative test where one volume mount is absent.
Added a negative 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.
LGTM
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.
LGTM Thanks for looking into this!!!!!
Volume was only checked on volume-mounts of main container.
In the case where volumes were defined for use on init-containers or additional-containers it was not added the list of volumes.
Now the volume is checked and added after all containers are known.
Fixes #454