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

feat(operatorinstaller): add initial deployment strategy #40

Merged
merged 13 commits into from
Sep 15, 2017

Conversation

ericavonb
Copy link
Contributor

Add beginning of operator install strategy logic by adding the "deployment" strategy.

import (
"encoding/json"
"fmt"
"github.com/coreos-inc/operator-client/pkg/client"
Copy link
Contributor

Choose a reason for hiding this comment

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

import (
   // std imports

  // third party imports

  // local imports
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the go fmt not really fix this?

Copy link
Contributor

@jzelinskie jzelinskie Sep 8, 2017

Choose a reason for hiding this comment

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

goimports does iirc, but i personally don't run it on save because it has corner-cases, specifically the prometheus client lib confuses it a lot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return nil, err
}
crd := &v1beta1extensions.CustomResourceDefinition{}
_, _, err = scheme.Codecs.UniversalDecoder().Decode(data, nil, crd)
Copy link
Contributor

Choose a reason for hiding this comment

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

these libraries suck

They use io.Writer for encoding, but []byte for decoding instead of io.Reader.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8fe6dc4 so much in these packages is just so 😞 . Added comment that this is for testing things out, not production use cases.

jzelinskie
jzelinskie previously approved these changes Sep 8, 2017
schema/loader.go Outdated
@@ -7,6 +7,8 @@ import (
"k8s.io/client-go/kubernetes/scheme"
)

// LoadCRDFromFile is a utility function for loading the CRD schemas.
// !!! WARNING !!! Not recommended for production use
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the case, we could put it in a test file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's where it was originally but it doesn't really belong in any of the packages being tested, so I put it in it's own thing for now.

@jzelinskie
Copy link
Contributor

Looks like there's a vendoring issue with CI

}

func deploymentFromUnstructured(d *unstructured.Unstructured) (*v1beta1.Deployment, error) {
data, err := json.Marshal(d)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason we can't cast to a deployment here?


mockClient := client.NewMockInterface(ctrl)

mockClient.EXPECT().
Copy link
Member

Choose a reason for hiding this comment

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

Should test the error case too

@ecordell ecordell force-pushed the ericavonb/ALM-111/alm-code-skeleton-operatorversion branch 7 times, most recently from 846357c to a7daa52 Compare September 15, 2017 14:24
@ecordell ecordell force-pushed the ericavonb/ALM-111/alm-code-skeleton-operatorversion branch from a7daa52 to 16065f9 Compare September 15, 2017 19:07
@charltonaustin charltonaustin force-pushed the ericavonb/ALM-111/alm-code-skeleton-operatorversion branch from 16065f9 to ee6916f Compare September 15, 2017 19:19
@ecordell ecordell force-pushed the ericavonb/ALM-111/alm-code-skeleton-operatorversion branch from ee6916f to dabc0e7 Compare September 15, 2017 21:50
Copy link
Member

@ecordell ecordell left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@charltonaustin charltonaustin left a comment

Choose a reason for hiding this comment

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

LGTM

@ecordell ecordell merged commit c810286 into master Sep 15, 2017
@jzelinskie jzelinskie deleted the ericavonb/ALM-111/alm-code-skeleton-operatorversion branch September 17, 2017 18:05
njhale pushed a commit to njhale/operator-lifecycle-manager that referenced this pull request Sep 10, 2018
…M-111/alm-code-skeleton-operatorversion

feat(operatorinstaller): add initial deployment strategy
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.

4 participants