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

secret objects & tests #13

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

VSharapov
Copy link

No description provided.

Copy link

@rrati rrati left a comment

Choose a reason for hiding this comment

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

Make all files through gofmt and fix formatting and remove commented out code in addition to changes.

}

// convertSecretType converts from a kubernetes SecretType
func convertSecretType(secret v1.SecretType) (SecretType, error) {
Copy link

Choose a reason for hiding this comment

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

Rename this in the same vain as other conversion functions (ie fromKubeSecreTypetV1). Name them after the Kubernetes object name and not the mantle one (if there's a difference). Keep in mind that there will likely be multiple conversion functions for multiple API versions, so name functions with that in mind.

v1 "k8s.io/api/core/v1"
)

type SecretWrapper struct {
Copy link

Choose a reason for hiding this comment

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

REmove this. Wrappers aren't needed in mantle

SecretType SecretType `json:"type,omitempty"`
}

type SecretType string
Copy link

Choose a reason for hiding this comment

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

Make this an int and all definitions of it using iota

return string(s)
}

func CompareSecretTypes(secret1, secret2 interface{}) (bool, error) {
Copy link

Choose a reason for hiding this comment

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

This function shouldn't be needed anymore or if it is, modified to work with secret type being an int. Also seems more like a test function, so move into test file and don't export

SecretTypeTLS SecretType = "kubernetes.io/tls"
)

func (s SecretType) ToString() string {
Copy link

Choose a reason for hiding this comment

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

Function not needed

SecretTypeDockerConfigJson SecretType = "kubernetes.io/dockerconfigjson"
SecretTypeBasicAuth SecretType = "kubernetes.io/basic-auth"
SecretTypeSSHAuth SecretType = "kubernetes.io/ssh-auth"
SecretTypeTLS SecretType = "kubernetes.io/tls"
Copy link

Choose a reason for hiding this comment

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

Missing the full set of types from kubernetes. SecretTypeBootstrapToken is missing

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// map[SecretName]V1SecretName
Copy link

Choose a reason for hiding this comment

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

Remove commented out code

kubeType := reflect.TypeOf(kubeObj)
expectedType := reflect.TypeOf(tc.expectedObj)
if kubeType != expectedType {
t.Errorf("wrong api version, got %s expected %s", kubeType, expectedType)
Copy link

Choose a reason for hiding this comment

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

Put the tc description at the head of the error message (ie "%s: ...)

expectedObj := reflect.TypeOf(&Secret{})
objType := reflect.TypeOf(obj)
if expectedObj != objType {
t.Errorf("expected %s got %s", expectedObj, objType)
Copy link

Choose a reason for hiding this comment

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

Put tc description at head of error message (ie "%s: ...")

}

// revertSecretType converts to a kubernetes SecretType
func revertSecretType(secret SecretType) (v1.SecretType, error) {
Copy link

Choose a reason for hiding this comment

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

Rename function as mentioned earlier

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.

3 participants