-
Notifications
You must be signed in to change notification settings - Fork 236
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
Grand Christmas Patch #479
Conversation
var jenkins = v1alpha2.Jenkins{ | ||
Spec: v1alpha2.JenkinsSpec{ | ||
Master: v1alpha2.JenkinsMaster{ | ||
Containers: []v1alpha2.Container{ | ||
{ | ||
Env: []corev1.EnvVar{ | ||
}, | ||
ReadinessProbe: &corev1.Probe{ | ||
Handler: corev1.Handler{ | ||
HTTPGet: &corev1.HTTPGetAction{ | ||
}, | ||
}, | ||
}, | ||
LivenessProbe: &corev1.Probe{ | ||
Handler: corev1.Handler{ | ||
HTTPGet: &corev1.HTTPGetAction{ | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please run go fmt
in this folder, i think there are additional unnecessary indents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eagle eye :D
jenkinsOptsEnv := envs[key] | ||
jenkinsOptsWithDashes := jenkinsOptsEnv.Value | ||
if len(jenkinsOptsWithDashes) == 0 { | ||
return nil | ||
} | ||
|
||
jenkinsOptsWithEqOperators := strings.Split(jenkinsOptsWithDashes, " ") | ||
|
||
for _, vx := range jenkinsOptsWithEqOperators { | ||
opt := strings.Split(vx, "=") | ||
jenkinsOpts[strings.ReplaceAll(opt[0], "--", "")] = opt[1] | ||
} | ||
|
||
return jenkinsOpts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see some magic here. are you sure that cannot be simpler?
} else if err != nil { | ||
return "", nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't use else if
, if you return something in first condition
This code looks good, but I think it can be simpler
runningInCluster, err := isRunningInCluster();
if !runningInCluster {
return kubernetesClusterDomain, nil
}
if err != nil {
return "", nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's up to you, which version you choose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see it's not my part of the code, but you're right. It needs a refactor.
|
||
suffix := "" | ||
if prefix, ok := resources.GetJenkinsOpts(*jenkins)["prefix"]; ok { | ||
suffix = prefix | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this piece of code in some places, maybe it's good idea to export it to dedicated function? (DRY)
Happy holidays, all. I've been trying to keep track of the state of the operator projects, sorry to drop in here but was wondering if we're to see a 0.4.1 or 0.5 release soon? |
Fixed helm not creating role permissions #424
Added ability to use custom k8s cluster domain #461
Fixed propagation of JENKINS_OPTS to seed-job agent #462
Fixed updating last backup #421
Also updated plugins.