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

Respect service type and make clusterIP default #257

Merged
merged 6 commits into from
Sep 20, 2022

Conversation

Jasstkn
Copy link
Member

@Jasstkn Jasstkn commented Aug 1, 2022

Signed-off-by: Jasstkn [email protected]

What this PR does / why we need it:

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

Special notes for your reviewer:

see also PR with documentation update: litmuschaos/litmus-docs#199

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md

Jasstkn added a commit to Jasstkn/litmus-docs that referenced this pull request Aug 1, 2022
Update documentation with this change litmuschaos/litmus-helm#257.
Add more details about connecting via NodePort and ClusterIP
Jasstkn added a commit to Jasstkn/litmus-docs that referenced this pull request Aug 1, 2022
Update documentation with this change litmuschaos/litmus-helm#257.
Add more details about connecting via NodePort and ClusterIP

Signed-off-by: Jasstkn <[email protected]>
@dchourasia
Copy link

@Jasstkn thanks a lot for a quick turn around, really appreciate it!
Can you please make the same change for backend server service (server-svc.yaml) as well, this will enable us to create/execute litmus workflows using server APIs directly (without going to UI)

Ingress.yaml already has a rule for backend server service as well.

@Jasstkn
Copy link
Member Author

Jasstkn commented Aug 2, 2022

@Jasstkn thanks a lot for a quick turn around, really appreciate it! Can you please make the same change for backend server service (server-svc.yaml) as well, this will enable us to create/execute litmus workflows using server APIs directly (without going to UI)

Ingress.yaml already has a rule for backend server service as well.

I'll take a look into that too, thanks

@Jasstkn
Copy link
Member Author

Jasstkn commented Aug 2, 2022

/run-e2e-all
Test Status: The e2e test has been started please wait for the results ...


Experiment Result Runtime

Test Failed: Some tests are failed please check
Run ID: 2783379804

@Jasstkn
Copy link
Member Author

Jasstkn commented Aug 2, 2022

@imrajdas hi. could you take a look into this PR please?

@dchourasia
Copy link

dchourasia commented Aug 2, 2022

@Jasstkn thank you for these changes, but I see some breaking changes in server-svc.yaml (this file belongs to backend server service and not graphql (mongo) service:

Can you please check following list:

  1. line # 14
    Expected: type: {{ .Values.portal.server.service.type }}
    Actual : type: {{ .Values.portal.server.graphqlServer.service.type }}

  2. line # 9
    Expected: {{- with .Values.portal.server.service.annotations }}
    Actual : {{- with .Values.portal.server.graphqlServer.service.annotations }}

Appreciate the efforts!

@Jasstkn
Copy link
Member Author

Jasstkn commented Aug 2, 2022

@Jasstkn thank you for these changes, but I see some breaking changes in server-svc.yaml (this file belongs to backend server service and not graphql (mongo) service:

Can you please check following list:

  1. line # 14
    Expected: type: {{ .Values.portal.server.service.type }}
    Actual : type: {{ .Values.portal.server.graphqlServer.service.type }}
  2. line # 9
    Expected: {{- with .Values.portal.server.service.annotations }}
    Actual : {{- with .Values.portal.server.graphqlServer.service.annotations }}

Appreciate the efforts!

it's not really a breaking-breaking change but it makes sense to manage service type per actual service. This change updates the general logic. You can see graphql's ports in the service template as well.
Could you elaborate a bit what do you mean?

@dchourasia
Copy link

@Jasstkn I see, yes this perfectly makes sense to have use separate fields for each service type. I was confused if there is any use of portal.server.service.type anymore.

So yes, not a breaking change, we will update our values.yaml accordingly.
thank you!

@Jasstkn
Copy link
Member Author

Jasstkn commented Aug 3, 2022

FYI: I'll write additional documentation about upgrade process to the README later today.

@Jasstkn Jasstkn force-pushed the 256-respect-service-type branch from c86536c to 0261653 Compare August 9, 2022 18:12
@Jasstkn
Copy link
Member Author

Jasstkn commented Aug 9, 2022

@imrajdas @ispeakc0de hi folks. Could you review this MR? I added upgrade notes as it's good practice to have some docs for users so they know how to upgrade their chart.

@Jasstkn Jasstkn force-pushed the 256-respect-service-type branch from 07dfa37 to 0975cb6 Compare August 21, 2022 10:00
Signed-off-by: Jasstkn <[email protected]>
@Jasstkn Jasstkn force-pushed the 256-respect-service-type branch from 0975cb6 to 44d3eb1 Compare August 21, 2022 10:02
@Jasstkn
Copy link
Member Author

Jasstkn commented Sep 15, 2022

@Jonsy13 hey! thanks for approving it. could you help me with merging this PR and update for docs: litmuschaos/litmus-docs#199?

@Jonsy13
Copy link
Contributor

Jonsy13 commented Sep 15, 2022

Hi @Jasstkn , Changes looks good to me. Gave a comment on docs PR.
Adding @imrajdas for review as well.

@imrajdas
Copy link
Member

@Jasstkn - Circleci checks are pending. Can you update this PR?

@Jonsy13
Copy link
Contributor

Jonsy13 commented Sep 20, 2022

Hi @Jasstkn Should i merge this PR?

@Jasstkn
Copy link
Member Author

Jasstkn commented Sep 20, 2022

Hi @Jasstkn Should i merge this PR?

merging this one. thanks!

fyi: docs PR is also updated.

@Jasstkn Jasstkn merged commit cf6befd into litmuschaos:master Sep 20, 2022
Jonsy13 pushed a commit to litmuschaos/litmus-docs that referenced this pull request Sep 21, 2022
* Update installation documentation

Update documentation with this change litmuschaos/litmus-helm#257.
Add more details about connecting via NodePort and ClusterIP

Signed-off-by: Jasstkn <[email protected]>

* Fix docs

Signed-off-by: Maria Kotlyarevskaya <[email protected]>

Signed-off-by: Jasstkn <[email protected]>
Signed-off-by: Maria Kotlyarevskaya <[email protected]>
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.

Litmus does not honor service.type field when ingress is enabled
6 participants