Add variable to enable to specify the name of port in the service #801

sonali-rajput · 2023-03-06T18:43:24Z

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:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

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

Describe any user facing changes here, or delete this block.

Examples of user facing changes:
- API changes
- Bug fixes
- Any changes in behavior

sonali-rajput · 2023-03-07T06:38:57Z

@brokenpip3 Can you please suggest me the changes I need to make in the test?

brokenpip3 · 2023-03-07T22:40:59Z

Hi @sonali-rajput thanks for your contribution!

@brokenpip3 Can you please suggest me the changes I need to make in the test?

You can just merge/rebase master here.

brokenpip3 · 2023-03-07T22:48:00Z

chart/jenkins-operator/templates/operator.yaml

@@ -26,7 +26,7 @@ spec:
          image: {{ .Values.operator.image }}
          imagePullPolicy: {{ .Values.operator.imagePullPolicy }}
          ports:
-            - name: http
+            - name: {{ .Values.portName }}


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 :)

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.

@brokenpip3 Is that right?

iamrajiv

@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.

brokenpip3

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 :)

brokenpip3 · 2023-03-16T22:22:14Z

main.go

@@ -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.")


why we should specify the port name in the commandline options?

brokenpip3 · 2023-03-16T22:22:22Z

main.go

@@ -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}


brokenpip3 · 2023-03-16T22:30:48Z

pkg/client/jenkins.go

@@ -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) {


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

@brokenpip3 I have made the required changes. Can you please review it? (sorry for the late reply I was busy preparing for GSoC'23)

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 :)

Okay I will make the changes.

sonali-rajput · 2023-05-03T08:44:29Z

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?

brokenpip3 · 2023-05-03T21:34:42Z

yup unfortunately when you enabled the workflow from the fork all the workflow will be run, not only the e2e tests one :)
Are you actively working on this PR? I saw some new commits

brokenpip3 · 2023-05-03T21:39:35Z

I took a look on the latest changes, looks good 👍
You only miss one thing: in the service.go you need to specify the default value http unless the user will provide another name.
Last bit: can you add a bats test that will double check if the port name is the right one? If need help let me know I can push that change as soon you have modified the service.go :)

sonali-rajput · 2023-05-04T10:02:30Z

I took a look on the latest changes, looks good 👍 You only miss one thing: in the service.go you need to specify the default value http unless the user will provide another name. Last bit: can you add a bats test that will double check if the port name is the right one? If need help let me know I can push that change as soon you have modified the service.go :)

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 git reset --hard then forced push the changes. But it is not the right way to do this right? Can you please help me here?

brokenpip3 · 2023-05-08T21:59:45Z

I took a look on the latest changes, looks good +1 You only miss one thing: in the service.go you need to specify the default value http unless the user will provide another name. Last bit: can you add a bats test that will double check if the port name is the right one? If need help let me know I can push that change as soon you have modified the service.go :)

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 git reset --hard then forced push the changes. But it is not the right way to do this right? Can you please help me here?

whatever git flow you followed the git history looks good :)
don't worry about the bats test, I will add the specific test

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 🎉

sonali-rajput · 2023-05-14T01:59:29Z

I took a look on the latest changes, looks good +1 You only miss one thing: in the service.go you need to specify the default value http unless the user will provide another name. Last bit: can you add a bats test that will double check if the port name is the right one? If need help let me know I can push that change as soon you have modified the service.go :)

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 git reset --hard then forced push the changes. But it is not the right way to do this right? Can you please help me here?

whatever git flow you followed the git history looks good :) don't worry about the bats test, I will add the specific test

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.

brokenpip3 · 2023-05-17T22:12:10Z

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 :)

brokenpip3 · 2023-06-10T08:18:13Z

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

…nsci#841)

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>

sonali-rajput · 2023-06-10T11:30:05Z

hi @sonali-rajput could you please merge master branch in your branch? I want to see the tests heavy_check_mark so we can finally merge the PR for the upcoming v0.8.0, thanks a lot!

Is that alright? I have rebased master on my branch

brokenpip3 reviewed Mar 7, 2023

View reviewed changes

iamrajiv suggested changes Mar 11, 2023

View reviewed changes

brokenpip3 reviewed Mar 16, 2023

View reviewed changes

Add port name in the service

0d24d95

sonali-rajput force-pushed the opertaor#736 branch from b202927 to 0d24d95 Compare April 4, 2023 18:13

Add default port name for service

67ecd35

sonali-rajput force-pushed the opertaor#736 branch from 0ac6c98 to 67ecd35 Compare April 23, 2023 06:29

sonali-rajput force-pushed the opertaor#736 branch from 90a3635 to 67ecd35 Compare May 4, 2023 09:48

Update service for default port name

4dabb60

brokenpip3 and others added 11 commits June 10, 2023 11:27

chore: update to 2.387.2-lts and base plugins (jenkinsci#811)

9f82b5e

chore: bump version to v0.8.0-beta (jenkinsci#814)

5323e2b

- add dependabot - add codespell - update operator version in helm chart

Release v0.8.0-beta Helm Chart (jenkinsci#825)

196b165

Co-authored-by: brokenpip3 <brokenpip3@users.noreply.github.com>

chore(doc): Update links in README (jenkinsci#829)

5c95fa4

chore: Update git ls-files cmd with missing arg (jenkinsci#832)

4737e9c

chore: update to latest lts, update plugins, remove devbots, add devc…

2e50336

…ontainer (jenkinsci#835) Update to the latest lts Update plugins Remove devbots Initial devcontainer config

feat(repo): add stale bot (jenkinsci#836)

55b7f0d

feat: use zstd instead of gz by default, fix the backup script (jenki…

1412754

…nsci#841)

Fix: add tollerations again (jenkinsci#842)

34566e3

Signed-off-by: tombokombo <tombo@sysart.tech> Co-authored-by: Tomas Hulata <tombo@sysart.tech>

feat: attempt to move restore before seed job creation (jenkinsci#844)

e92f91e

chore: add v0.8.0 doc (jenkinsci#845)

89bebb0

brokenpip3 and others added 7 commits June 10, 2023 11:27

fix: backup trimming, only selecting zstd file (jenkinsci#846)

79ec248

Bump docker/login-action from 1 to 2 (jenkinsci#819)

7c949b9

Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump actions/checkout from 2 to 3 (jenkinsci#815)

9f04ae2

Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bump peter-evans/create-pull-request from 3 to 5 (jenkinsci#817)

9936d50

Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

fix hugo and backup version, fix backup ci (jenkinsci#847)

363b277

fix: hugo and backup ci again (jenkinsci#848)

9aa8642

chore: prearing version bumpt to v0.8.0-beta2 (jenkinsci#849)

8db6fd5

github-actions bot added the stale label Aug 10, 2023

github-actions bot closed this Aug 18, 2023

brokenpip3 reopened this Aug 18, 2023

github-actions bot removed the stale label Aug 19, 2023

github-actions bot added the stale label Oct 18, 2023

github-actions bot closed this Oct 25, 2023

brokenpip3 reopened this Oct 25, 2023

brokenpip3 added not-stale and removed stale labels Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add variable to enable to specify the name of port in the service #801

Add variable to enable to specify the name of port in the service #801

sonali-rajput commented Mar 6, 2023 •

edited

Loading

sonali-rajput commented Mar 7, 2023

brokenpip3 commented Mar 7, 2023

brokenpip3 Mar 7, 2023

sonali-rajput Mar 9, 2023

sonali-rajput Mar 11, 2023

iamrajiv left a comment

brokenpip3 left a comment

brokenpip3 Mar 16, 2023

brokenpip3 Mar 16, 2023

brokenpip3 Mar 16, 2023

sonali-rajput Apr 4, 2023

brokenpip3 Apr 6, 2023

sonali-rajput Apr 7, 2023

sonali-rajput commented May 3, 2023

brokenpip3 commented May 3, 2023

brokenpip3 commented May 3, 2023

sonali-rajput commented May 4, 2023 •

edited

Loading

brokenpip3 commented May 8, 2023

sonali-rajput commented May 14, 2023

brokenpip3 commented May 17, 2023

brokenpip3 commented Jun 10, 2023

sonali-rajput commented Jun 10, 2023

Add variable to enable to specify the name of port in the service #801

Are you sure you want to change the base?

Add variable to enable to specify the name of port in the service #801

Conversation

sonali-rajput commented Mar 6, 2023 • edited Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

sonali-rajput commented Mar 7, 2023

brokenpip3 commented Mar 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamrajiv left a comment

Choose a reason for hiding this comment

brokenpip3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonali-rajput commented May 3, 2023

brokenpip3 commented May 3, 2023

brokenpip3 commented May 3, 2023

sonali-rajput commented May 4, 2023 • edited Loading

brokenpip3 commented May 8, 2023

sonali-rajput commented May 14, 2023

brokenpip3 commented May 17, 2023

brokenpip3 commented Jun 10, 2023

sonali-rajput commented Jun 10, 2023

sonali-rajput commented Mar 6, 2023 •

edited

Loading

sonali-rajput commented May 4, 2023 •

edited

Loading