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

Allow user to configure pod hostAliases #4787

Conversation

heylongdacoder
Copy link
Contributor

@heylongdacoder heylongdacoder commented May 13, 2022

Fixes #3488, Fixes #4298

Signed-off-by: heylongdacoder heylongdacoder@gmail.com

Description

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request.
If it fixes a bug or resolves a feature request, be sure to link to that issue.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

  • CHANGE (fix or feature that would cause existing functionality to not work as expected)
  • FEATURE (non-breaking change which adds functionality)
  • BUGFIX (non-breaking change which fixes an issue)
  • ENHANCEMENT (non-breaking change which improves existing functionality)
  • NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Please put a one-line changelog entry below. This will be copied to the changelog file during the release process.

Added hostAliases field to Prometheus, ThanosRuler and Alertmanager CRD

@heylongdacoder heylongdacoder requested a review from a team as a code owner May 13, 2022 11:05
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

could you add unit tests for Prometheus and Alertmanager? thanks!

pkg/apis/monitoring/v1/thanos_types.go Show resolved Hide resolved
pkg/apis/monitoring/v1/types.go Show resolved Hide resolved
pkg/apis/monitoring/v1/types.go Outdated Show resolved Hide resolved
@heylongdacoder heylongdacoder force-pushed the feature/pod-hostaliases branch 2 times, most recently from 12c6ba8 to c29146f Compare May 16, 2022 02:12
@heylongdacoder
Copy link
Contributor Author

@simonpasquier I have added the test case for Prometheus and Alertmanager, but I need help on the e2e build error. Currently I am not familiar with the controller-gen, could you please help me on this?

@simonpasquier
Copy link
Contributor

I believe that to make it work, we have to declare our own hostAlias struct instead of importing it from the v1 package.

@heylongdacoder
Copy link
Contributor Author

@simonpasquier thanks! I think I got what you meant. Let me study about the controller-gen before continuing this :)

@heylongdacoder heylongdacoder force-pushed the feature/pod-hostaliases branch 3 times, most recently from df59cb0 to 6ee2111 Compare June 2, 2022 11:30
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

I just a few minor comments otherwise this is looking great!

pkg/k8sutil/k8sutil.go Outdated Show resolved Hide resolved
pkg/k8sutil/k8sutil.go Outdated Show resolved Hide resolved
pkg/alertmanager/statefulset_test.go Outdated Show resolved Hide resolved
Thanos Pods.

Signed-off-by: heylongdacoder <heylongdacoder@gmail.com>
Copy link
Contributor

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

Thanks!

@simonpasquier simonpasquier enabled auto-merge (squash) June 2, 2022 15:27
@heylongdacoder
Copy link
Contributor Author

@simonpasquier thanks for the review, I learnt something :)

@simonpasquier simonpasquier merged commit e455740 into prometheus-operator:main Jun 2, 2022
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.

Add hostAlias support in Prometheus CRD Add an option to add hostAlias for alert manager
2 participants