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.

Sign up for GitHub

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

Jump to bottom

feat(controller): add support for mute timing #1817

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(controller): add support for mute timing #1817

wants to merge 1 commit into from
+1,858 −0

Conversation

chenlujjj
Copy link
Contributor

To resolve: #1723

@chenlujjj chenlujjj marked this pull request as draft January 11, 2025 13:31
@chenlujjj chenlujjj force-pushed the feat/mute-timing branch 7 times, most recently from 2420950 to f0b9e9b Compare January 11, 2025 15:07
@chenlujjj chenlujjj marked this pull request as ready for review January 11, 2025 15:07
@chenlujjj
Copy link
Contributor Author

tested in local:

---
apiVersion: grafana.integreatly.org/v1beta1
kind: GrafanaMuteTiming
metadata:
  name: mutetiming-sample
spec:
  instanceSelector:
    matchLabels:
      dashboards: "grafana"
  name: mutetiming-sample
  editable: false
  time_intervals:
    - times:
        - start_time: "00:00"
          end_time: "06:00"
      weekdays: ["saturday", "sunday"]
      days_of_month: ["1", "10"]
      location: "Asia/Shanghai"

result:
image

@theSuess
Copy link
Member

Thanks for the PR! I'll take a look at this soon. It's too big of a change for the next release, but we'll get it out soon now that the holiday season is over

@theSuess theSuess added this to the v5.17.0 milestone Jan 13, 2025
@theSuess theSuess added the feature this PR introduces a new feature label Jan 15, 2025
Baarsgaard
Baarsgaard suggested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@Baarsgaard Baarsgaard left a comment

Choose a reason for hiding this comment

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

These along with a rebase onto master should be mostly it.

api/v1beta1/grafanamutetiming_types.go Show resolved Hide resolved
api/v1beta1/grafanamutetiming_types.go Outdated
// Whether to enable or disable editing of the mute timing in Grafana UI
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="spec.editable is immutable"
// +optional
Editable *bool `json:"editable,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

@theSuess @weisdd What was the decision on *bool, is plain bool preferred with default true?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have a definite decision. With new resources, I think we should go with a plain bool (& default to true)

controllers/grafanamutetiming_controller.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the documentation Issues relating to documentation, missing, non-clear etc. label Jan 22, 2025
theSuess
theSuess requested changes Jan 23, 2025
View reviewed changes
Copy link
Member

@theSuess theSuess left a comment

Choose a reason for hiding this comment

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

This looks great - sorry for the late review!

Once the minor comments I left are addressed, I'm happy to merge this. Will definitely make it into the next release

controllers/grafanamutetiming_controller.go Outdated Show resolved Hide resolved
controllers/grafanamutetiming_controller.go Outdated Show resolved Hide resolved
api/v1beta1/grafanamutetiming_types.go Outdated Show resolved Hide resolved
api/v1beta1/grafanamutetiming_types.go Outdated
// Whether to enable or disable editing of the mute timing in Grafana UI
// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="spec.editable is immutable"
// +optional
Editable *bool `json:"editable,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have a definite decision. With new resources, I think we should go with a plain bool (& default to true)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Baarsgaard Baarsgaard Baarsgaard requested changes

@theSuess theSuess theSuess requested changes

@NissesSenap NissesSenap Awaiting requested review from NissesSenap NissesSenap is a code owner

@weisdd weisdd Awaiting requested review from weisdd weisdd is a code owner

@ishanjainn ishanjainn Awaiting requested review from ishanjainn ishanjainn is a code owner

@hubeadmin hubeadmin Awaiting requested review from hubeadmin hubeadmin is a code owner

@pb82 pb82 Awaiting requested review from pb82 pb82 is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
documentation Issues relating to documentation, missing, non-clear etc. feature this PR introduces a new feature
Projects
None yet
Milestone
v5.17.0
Development

Successfully merging this pull request may close these issues.

Feature: Provision mute time intervals trough the operator
3 participants
@chenlujjj @theSuess @Baarsgaard