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

fix: (helm) service template now respects defined port and is only templated if prometheus is enabled #101

Merged
merged 1 commit into from
May 31, 2022

Conversation

rafaribe
Copy link
Contributor

I've noticed that the service port as defined in values.yaml is not respected. As such I created a pull request to respect the value and, by doing so, noticed that the service is only used for metrics. Therefore the service can also be only templated if prometheus.enabled is true.

Let me know if anything is wrong.

@rafaribe rafaribe requested a review from a team as a code owner May 29, 2022 19:07
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Hi @rafaribe and thank you for the PR. I got a small comment, but good catch!

install/kubernetes/templates/service.yaml Outdated Show resolved Hide resolved
@kaworu kaworu added the kind/enhancement This improves or streamlines existing functionality label May 30, 2022
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

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

Thanks!

@kaworu
Copy link
Member

kaworu commented May 30, 2022

@rafaribe Please squash the commits into one and sign-off the resulting commit (see https://docs.cilium.io/en/v1.11/contributing/development/contributing_guide/#developer-s-certificate-of-origin).

@rafaribe rafaribe force-pushed the main branch 2 times, most recently from 181df2c to 1af8247 Compare May 30, 2022 12:29
@rafaribe
Copy link
Contributor Author

@rafaribe Please squash the commits into one and sign-off the resulting commit (see https://docs.cilium.io/en/v1.11/contributing/development/contributing_guide/#developer-s-certificate-of-origin).

There you go.

Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Thanks!

@kkourt
Copy link
Contributor

kkourt commented May 30, 2022

There seem to be two errors in the static checks:

  Signed-off-by: Rafael Ribeiro <rafael.ntw@gmail.com>}
  Running on 1af8247a1e04df7e76846266ff48714db3c7860a
  Error: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)
  
  
  NOTE: For some of the reported defects, checkpatch may be able to
        mechanically convert to the typical style using --fix or --fix-inplace.
  
  "[PATCH]  fix: (helm) service template now respects defined port and" has style problems, please review.
  
  NOTE: Ignored message types: C99_COMMENTS C99_COMMENT_TOLERANCE COMMIT_LOG_LONG_LINE COMMIT_MESSAGE COMPLEX_MACRO CONSTANT_CONVERSION CONST_STRUCT EMAIL_SUBJECT FILE_PATH_CHANGES FROM_SIGN_OFF_MISMATCH GIT_COMMIT_ID JIFFIES_COMPARISON LEADING_SPACE LONG_LINE_COMMENT MACRO_WITH_FLOW_CONTROL MULTISTATEMENT_MACRO_USE_DO_WHILE NOT_UNIFIED_DIFF PREFER_DEFINED_ATTRIBUTE_MACRO PRINTK_WITHOUT_KERN_LEVEL TRAILING_SEMICOLON TRAILING_STATEMENTS VOLATILE
  
  NOTE: If any of the errors are false positives, please report
        them to the maintainer, see CHECKPATCH in MAINTAINERS.
  Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 155)

I don't understand why the Signed-off-by failure is there, the lines seem identical to me.

@rafaribe
Copy link
Contributor Author

There seem to be two errors in the static checks:

  Signed-off-by: Rafael Ribeiro <rafael.ntw@gmail.com>}
  Running on 1af8247a1e04df7e76846266ff48714db3c7860a
  Error: ERROR:MISSING_SIGN_OFF: Missing Signed-off-by: line(s)
  
  
  NOTE: For some of the reported defects, checkpatch may be able to
        mechanically convert to the typical style using --fix or --fix-inplace.
  
  "[PATCH]  fix: (helm) service template now respects defined port and" has style problems, please review.
  
  NOTE: Ignored message types: C99_COMMENTS C99_COMMENT_TOLERANCE COMMIT_LOG_LONG_LINE COMMIT_MESSAGE COMPLEX_MACRO CONSTANT_CONVERSION CONST_STRUCT EMAIL_SUBJECT FILE_PATH_CHANGES FROM_SIGN_OFF_MISMATCH GIT_COMMIT_ID JIFFIES_COMPARISON LEADING_SPACE LONG_LINE_COMMENT MACRO_WITH_FLOW_CONTROL MULTISTATEMENT_MACRO_USE_DO_WHILE NOT_UNIFIED_DIFF PREFER_DEFINED_ATTRIBUTE_MACRO PRINTK_WITHOUT_KERN_LEVEL TRAILING_SEMICOLON TRAILING_STATEMENTS VOLATILE
  
  NOTE: If any of the errors are false positives, please report
        them to the maintainer, see CHECKPATCH in MAINTAINERS.
  Error: ERROR:CUSTOM: Please avoid long commit subjects (max: 75, found: 155)

I don't understand why the Signed-off-by failure is there, the lines seem identical to me.

I've issued the commit again using git commit -S -s -m "<message>" Should be fixed now.

@kkourt
Copy link
Contributor

kkourt commented May 30, 2022

I've issued the commit again using git commit -S -s -m "<message>" Should be fixed now.

Ah, thanks! above seems to fix the signing issue. Could you also change the commit message so that the first line it's shorter?

How about:

helm: fix metrics template

template now respects defined prometheus port (tetragon.prometheus.port) value.
Also, it only adds the service if prometheus is enabled
(tetragon.prometheus.enabled).

template now respects defined prometheus port (tetragon.prometheus.port) value.
Also, it only adds the service if prometheus is enabled
(tetragon.prometheus.enabled).

Signed-off-by: Rafael Ribeiro <rafael.ntw@gmail.com>
Copy link
Contributor

@willfindlay willfindlay left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!!

@kkourt kkourt merged commit b4784e7 into cilium:main May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement This improves or streamlines existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants