Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

implement ContainerConfigFile of ContainerizedWorkload #276

Merged
merged 1 commit into from
Nov 6, 2020

Conversation

captainroy-hy
Copy link
Contributor

fix #235

Currently, "ContainerConfigFile " of ContainerizedWorkload will be translated to ConfigMap. Afterward, the newly created ConfigMap will be mounted to target Pod through Volume.
If Config.FromSecret is specified, it will just mount the target secret through volume instead of using ConfigMap.

  • add e2e-test
  • add unit test

Signed-off-by: roy wang seiwy2010@gmail.com

wonderflow
wonderflow previously approved these changes Nov 2, 2020
Copy link
Member

@wonderflow wonderflow left a comment

Choose a reason for hiding this comment

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

generally LGTM, please add some yaml examples for demo

ObjectMeta: metav1.ObjectMeta{
Name: cmName,
Namespace: cw.GetNamespace(),
},
Copy link
Member

Choose a reason for hiding this comment

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

please add ownerReference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. And I will add yaml examples.

}
for _, cm := range configMaps {
// always set the controller reference so that we can watch this configmap and it will be deleted automatically
if err := ctrl.SetControllerReference(workload, cm, r.Scheme); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

what will happen for this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will append the workload as an ownerRef to the configMap, maybe I should use the deployment instead of workload.

@wonderflow
Copy link
Member

add e2e-test

add unit test

Signed-off-by: roy wang <seiwy2010@gmail.com>
@captainroy-hy
Copy link
Contributor Author

maybe we need to add configmap and secret previlige here https://github.com/crossplane/oam-kubernetes-runtime/blob/master/charts/oam-kubernetes-runtime/templates/oam-controller.yaml#L24-L57

Yes, that's right. And I add configmaps privileges.

},
}
// pass through label and annotation from the workload to the configmap
util.PassLabelAndAnnotation(w, cm)
Copy link
Member

Choose a reason for hiding this comment

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

It seems no need to pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If so, I will remove it. The original concern is to stay in line with how the controller handles labels&annotations of other child resources.

Copy link
Member

Choose a reason for hiding this comment

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

ownerReference is a MUST have, the labels and annotations I don't have strong opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A Controller ownerReference is set here

@wonderflow wonderflow dismissed their stale review November 4, 2020 02:22

some other changes needed

@wonderflow wonderflow merged commit 7a6a94a into crossplane:master Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The "config" attribute is not working for ContainerizedWorkload
2 participants