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 kube apply action #58

Merged
merged 1 commit into from Feb 26, 2018
Merged

*: add kube apply action #58

merged 1 commit into from Feb 26, 2018

Conversation

hasbro17
Copy link
Contributor

Part [1/2] of #57

Added implementation for kube apply.

For the resourceVersion conflict case the only thing to do would be to fail the action, which would re-queue the key, retry the sync, and the Handler. The expectation is that calling Handle() again would give us an Action with the latest object.

@hasbro17
Copy link
Contributor Author

@hongchaodeng @fanminshi PTAL

object, err := k8sutil.RuntimeObjectFromUnstructured(unstructObj)
if err != nil {
logrus.Errorf("failed to get runtime object from unstructured: %v", err)
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the unstructured could not convert to runtime object, it seems like we can't handle it anyway. Why not just panic inside the method?

@hongchaodeng
Copy link
Contributor

LGTM after nit

return nil
}

// KubeApply will try to create the specified object or update it if it already exists
Copy link
Contributor

Choose a reason for hiding this comment

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

KubeApply will try to create -> KubeApply tries to create?

@fanminshi
Copy link
Contributor

lgtm after a small suggestion.

@hasbro17 hasbro17 merged commit 4538206 into operator-framework:master Feb 26, 2018
@hasbro17 hasbro17 deleted the haseeb/add-kube-apply branch February 26, 2018 19:36
@fanminshi fanminshi mentioned this pull request Feb 26, 2018
21 tasks
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

3 participants