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

Add EmptyDir volumes for new-app containers #3005

Closed
wants to merge 1 commit into from

Conversation

mnagy
Copy link
Contributor

@mnagy mnagy commented Jun 9, 2015

Modifies new-app so it defines volumes and volume mounts for containers based on volumes defined in docker images. Created volume sources are EmptyDir. Currently, the naming scheme is "containerName-volume-1".

For this to work and not break new-app with our images, #3037 must be merged first.

Example with json modified for brevity:

$ osc new-app --docker-image=openshift/mysql-55-centos7 -o json
{
    "kind": "List",
    "apiVersion": "v1",
    "metadata": {},
    "items": [
        {
            "kind": "DeploymentConfig",
            "apiVersion": "v1",
            "metadata": {
                "name": "mysql-55-centos7",
                "creationTimestamp": null
            },
            "spec": {
                "template": {
                    "spec": {
                        "volumes": [
                            {
                                "name": "mysql-55-centos7-volume-1",
                                "emptyDir": {},
                                "rbd": null
                            }
                        ],
                        "containers": [
                            {
                                "name": "mysql-55-centos7",
                                "image": "openshift/mysql-55-centos7:latest",
                                "volumeMounts": [
                                    {
                                        "name": "mysql-55-centos7-volume-1",
                                        "mountPath": "/var/lib/mysql/data"
                                    }
                                ]
                            }
                        ]
                    }
                }
            },
        }
    ]
}

Fixes #2929

@mfojtik
Copy link
Contributor

mfojtik commented Jun 10, 2015

[test]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/3088/)

@mnagy mnagy force-pushed the volume_empty_dir branch 2 times, most recently from 15e6d47 to 2fab0b4 Compare June 10, 2015 13:28
@mnagy
Copy link
Contributor Author

mnagy commented Jun 10, 2015

Added & modified tests, thanks @Kargakis for help! PTAL

template.Volumes = append(template.Volumes, kapi.Volume{
Name: v.Name,
VolumeSource: kapi.VolumeSource{
EmptyDir: &kapi.EmptyDirVolumeSource{Medium: ""},
Copy link
Contributor

Choose a reason for hiding this comment

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

Use this constant here

@mnagy
Copy link
Contributor Author

mnagy commented Jun 10, 2015

This can go in after review and #3037

@0xmichalis
Copy link
Contributor

LGTM on green

@smarterclayton smarterclayton modified the milestone: 1.0.0 Jun 11, 2015
@mnagy mnagy changed the title [WIP] Add EmptyDir volumes for new-app containers Add EmptyDir volumes for new-app containers Jun 11, 2015
@openshift-bot
Copy link
Contributor

Evaluated for origin up to 4ca7271

@mnagy
Copy link
Contributor Author

mnagy commented Jun 11, 2015

Only assets test fails, unrelated to this PR.

@smarterclayton your approval please?

} else {
srcType = "UNKNOWN"
}
got["volumes"] = append(got["volumes"], volume.Name+"♥"+srcType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unicode symbols can blow up user's editors. Let's avoid them in our codebase.

@mnagy
Copy link
Contributor Author

mnagy commented Jun 12, 2015

Closing in favour of #3098 since it encompasses this change and adds the warning.

@mnagy mnagy closed this Jun 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants