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

prometheus.operator.servicemonitors #3487

Merged
merged 15 commits into from
Apr 27, 2023
Merged

prometheus.operator.servicemonitors #3487

merged 15 commits into from
Apr 27, 2023

Conversation

captncraig
Copy link
Contributor

@captncraig captncraig commented Apr 6, 2023

Implements #3384

@captncraig captncraig changed the title Refactor podmonitors to make more reusable prometheus.operator.servicemonitors Apr 6, 2023
@captncraig
Copy link
Contributor Author

Implements #3384

@captncraig captncraig marked this pull request as ready for review April 18, 2023 14:39
@marctc
Copy link
Contributor

marctc commented Apr 26, 2023

Implements #3384

You'd need add that to the description of the PR in order to get the related issue closed


// Update implements component.Component.
func (c *Component) Update(args component.Arguments) error {
// TODO(jcreixell): Initialize manager here so we can return errors back early to the caller.
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT to address this TODO in this PR?

}

func NewCRDManager(opts component.Options, logger log.Logger, args *Arguments) *crdManager {
const (
KindPodMonitor string = "podMonitor"
Copy link
Contributor

Choose a reason for hiding this comment

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

we may want to define these constants in component.go (potentially with a type alias) and reference them here to prevent duplication and inconsistencies

func (c *crdManager) configureInformers(ctx context.Context, informers cache.Informers) error {
podMonitor := &promopv1.PodMonitor{}
var proto client.Object
Copy link
Contributor

Choose a reason for hiding this comment

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

would object or k8sObject be a more descriptive name?

Copy link
Contributor

@jcreixell jcreixell left a comment

Choose a reason for hiding this comment

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

overall LGTM, the refactoring makes sense. Added a few nits like replacing harcoded strings by top level constants with custom types. Not much more to add aside from what @marctc already mentioned

@captncraig captncraig enabled auto-merge (squash) April 27, 2023 14:57
@captncraig captncraig merged commit a7996cf into main Apr 27, 2023
@captncraig captncraig deleted the cmp_refactor branch April 27, 2023 16:09
thampiotr pushed a commit that referenced this pull request May 2, 2023
@github-actions github-actions bot added the frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed. label Feb 28, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
frozen-due-to-age Locked due to a period of inactivity. Please open new issues or PRs if more discussion is needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants