Skip to content
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

Merged

Conversation

corneil
Copy link
Contributor

@corneil corneil commented Aug 6, 2024

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

@corneil corneil requested a review from cppwfs August 6, 2024 11:37
@corneil corneil added this to the 2.9.5 milestone Aug 6, 2024
@corneil corneil requested a review from onobc August 6, 2024 14:11
@@ -250,10 +251,7 @@ PodSpec createPodSpec(AppDeploymentRequest appDeploymentRequest) {
podSpec.withTolerations(this.deploymentPropertiesResolver.getTolerations(deploymentProperties));

// only add volumes with corresponding volume mounts
Copy link
Contributor

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.

Copy link
Contributor Author

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}]}}]");
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@cppwfs
Copy link
Contributor

cppwfs commented Aug 7, 2024

One more request can we have a negative test? 🙏

@corneil corneil requested a review from cppwfs August 22, 2024 06:24
Added negative test where one volume mount is absent.
@corneil
Copy link
Contributor Author

corneil commented Aug 23, 2024

One more request can we have a negative test? 🙏

Added a negative test

Copy link
Contributor

@cppwfs cppwfs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cppwfs cppwfs added the type/forwardport Is a issue to track forward, use with branch/xxx label Aug 23, 2024
Copy link
Contributor

@cppwfs cppwfs left a 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!!!!!

@corneil corneil merged commit 65dbb7d into spring-cloud:main Aug 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/forwardport Is a issue to track forward, use with branch/xxx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Anyone who get trouble using property additional container & volumeMounts
2 participants