-
Notifications
You must be signed in to change notification settings - Fork 235
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 variable to enable to specify the name of port in the service #801
base: master
Are you sure you want to change the base?
Conversation
@brokenpip3 Can you please suggest me the changes I need to make in the test? |
Hi @sonali-rajput thanks for your contribution!
You can just merge/rebase master here. |
@@ -26,7 +26,7 @@ spec: | |||
image: {{ .Values.operator.image }} | |||
imagePullPolicy: {{ .Values.operator.imagePullPolicy }} | |||
ports: | |||
- name: http | |||
- name: {{ .Values.portName }} |
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.
In the original issue the request was to have the possibility of specifying the name of the port of the result jenkins pod not the operator.
This will require to add a serviceName here and more golang code, finally the helm chart change.
If you are looking to something more easy to do I can suggest you to take a look on the others good-first-issue
issue :)
If don't you can start with some changes and we can double check the code together :)
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 am new to all this but I'll try to resolve this issue as soon as I can and get back to you. Thank you for guiding me in this, I really appreciate that.
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.
@brokenpip3 Is that right?
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.
@sonali-rajput I suggest removing redundant checks and refactoring the code to improve readability and efficiency. The refactored code for validating Jenkins API connection settings looks like this:
func (j JenkinsAPIConnectionSettings) Validate() error {
if j.Port < 0 {
return errors.New("service port cannot be lower than 0")
}
if j.Hostname == "" {
return errors.New("empty hostname is not allowed. Please provide hostname")
}
if j.Portname == "" {
return errors.New("empty portname is not allowed. Please provide portname")
}
if j.Port > 0 && j.UseNodePort {
return errors.New("cannot use service port and nodePort at the same time. Please use port or nodePort")
}
return nil
}
This code checks for negative service port values first, then validates the hostname and portname fields for emptiness. Finally, it checks whether the service port and nodePort are both set, which is not allowed.
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.
Overall looks ok!
I left a couple of comments and also:
- we also need to add the helm chart template modifications to support the port name (we need to provide a default:
http
) - we also need to take care of the CRD part:
- chart/jenkins-operator/crds/jenkins-crd.yaml
- config/crd/bases/jenkins.io_jenkins.yaml
you can check this Allow to specify seed agent image #718 and this PR Add seedJobAgentImage into CRD #754 to compare with the changes we need there :)
main.go
Outdated
@@ -94,6 +94,7 @@ func main() { | |||
port := flag.Int("jenkins-api-port", 0, "The port on which Jenkins API is running. Note: If you want to use nodePort don't set this setting and --jenkins-api-use-nodeport must be true.") | |||
useNodePort := flag.Bool("jenkins-api-use-nodeport", false, "Connect to Jenkins API using the service nodePort instead of service port. If you want to set this as true - don't set --jenkins-api-port.") | |||
kubernetesClusterDomain := flag.String("cluster-domain", "cluster.local", "Use custom domain name instead of 'cluster.local'.") | |||
portName := flag.String("jenkins-api-portname", "default", "The port name given to the service.") |
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.
why we should specify the port name in the commandline options?
main.go
Outdated
@@ -158,7 +159,7 @@ func main() { | |||
go notifications.Listen(notificationEvents, events, mgr.GetClient()) | |||
|
|||
// validate jenkins API connection | |||
jenkinsAPIConnectionSettings := client.JenkinsAPIConnectionSettings{Hostname: *hostname, Port: *port, UseNodePort: *useNodePort} | |||
jenkinsAPIConnectionSettings := client.JenkinsAPIConnectionSettings{Hostname: *hostname, Port: *port, UseNodePort: *useNodePort, Portname: *portName} |
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.
same here
pkg/client/jenkins.go
Outdated
@@ -130,6 +131,10 @@ func (j JenkinsAPIConnectionSettings) Validate() error { | |||
return errors.New("empty hostname is now allowed. Please provide hostname") | |||
} | |||
|
|||
if (j.Portname == "" && j.Port > 0) || (j.Portname == "" && j.UseNodePort) { |
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.
We should avoid to force a port name, we don't need it to let the operator or jenkins run, we just need it in the CRDs to be added in the helm chart and consumed by the operator while creating the jenkins pod
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.
@brokenpip3 I have made the required changes. Can you please review it? (sorry for the late reply I was busy preparing for GSoC'23)
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.
Like this that field is not consumed by the operator while creating the jenkins pod, you need to add some code in pkg/configuration/base/resources/service.go
where also you need to add the default http
as port name if is not defined in the crd :)
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.
Okay I will make the changes.
b202927
to
0d24d95
Compare
0ac6c98
to
67ecd35
Compare
Hi @brokenpip3 I don't understand why the workflows have been running in my forked repository here https://github.com/sonali-rajput/kubernetes-operator/actions it should not be running in my repo unless I did it manually right? |
yup unfortunately when you enabled the workflow from the fork all the workflow will be run, not only the e2e tests one :) |
I took a look on the latest changes, looks good 👍 |
90a3635
to
67ecd35
Compare
Hi @brokenpip3 I have made the changes but I made a mistake earlier while committing the changes the commits done by you also appeared in my commit so I did |
whatever git flow you followed the git history looks good :) as soon this #835 will be merged in master merge/rebase master in your branch so we can fix the ci and finally we can merge this PR 🎉 |
Alright! @brokenpip3 😊 Thanks for helping me with the PR. |
Thanks to you! I will let you know when you can merge master in your branch so we can pass the tests and finally merge this PR :) |
hi @sonali-rajput could you please merge master branch in your branch? I want to see the tests ✔️ so we can finally merge the PR for the upcoming v0.8.0, thanks a lot! |
- add dependabot - add codespell - update operator version in helm chart
Co-authored-by: brokenpip3 <brokenpip3@users.noreply.github.com>
…ontainer (jenkinsci#835) Update to the latest lts Update plugins Remove devbots Initial devcontainer config
Signed-off-by: tombokombo <tombo@sysart.tech> Co-authored-by: Tomas Hulata <tombo@sysart.tech>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Is that alright? I have rebased master on my branch |
HI! This change would be quite helpful for a project I'm working on that involves the Prometheus operator that needs port names for its ServiceMonitor CRD. Are there any tasks for this PR that are outstanding that I can assist with? I'm happy to help where I can. |
Changes
Fixes #736
Add variable in the helm chart to enable to specify the name of port in the service.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS.
Release Notes