Bug 1822396: Update metric when Subscription is updated #1603
Conversation
openshift-ci-robot
added
bugzilla/severity-medium
|
@anik120: This pull request references Bugzilla bug 1822396, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test e2e-aws-olm |
2 similar comments
|
/test e2e-aws-olm |
|
/test e2e-aws-olm |
openshift-ci-robot
added
the
needs-rebase
anik120
force-pushed
the
subs_sync_metric_re-fix
branch
from
2583f2c to
447c11b
Compare
openshift-ci-robot
removed
the
needs-rebase
anik120
force-pushed
the
subs_sync_metric_re-fix
branch
from
447c11b to
c6e37ba
Compare
test/e2e/metrics_e2e_test.go
Outdated
| WithName("metric-subscription-for-update"), | ||
| WithChannel(alphaChannel), | ||
| WithPackage(testPackageName), | ||
| WithPackage(testPackageName), |
There was a problem hiding this comment.
It's harmless, but this line is duplicated.
| @@ -267,5 +267,7 @@ func UpdateSubsSyncCounterStorage(sub *olmv1alpha1.Subscription) { | |||
| counterValues.installedCSV = sub.Status.InstalledCSV | |||
| counterValues.pkg = sub.Spec.Package | |||
| counterValues.channel = sub.Spec.Channel | |||
|
|
|||
| subscriptionSyncCounters[sub.GetName()] = counterValues | |||
There was a problem hiding this comment.
I don't think it needs to block this bugfix, but there should really be some kind of synchronization around access to subscriptionSyncCounters. Would you be interested in making a separate PR with that change?
There was a problem hiding this comment.
@benluddy sure! I'm curious to see what you mean by synchronization, once we discuss that I'd be happy to open up a PR if needed.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: anik120, benluddy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retest |
2 similar comments
|
/retest |
|
/retest |
In operator-framework#1519, the subscription_sync_total metric was updated when the related subscription object was updated. However, the PR had a bug, in which the map used to store metric related information was not being updated when the metric was udpated. As a result of which any update after the first update was not removing the old metric. This PR fixes that bug.
anik120
force-pushed
the
subs_sync_metric_re-fix
branch
from
c6e37ba to
2b658ed
Compare
|
/test e2e-aws-console-olm |
|
/lgtm |
|
@anik120: All pull requests linked via external trackers have merged: operator-framework/operator-lifecycle-manager#1603, operator-framework/operator-lifecycle-manager#1519. Bugzilla bug 1822396 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Description of the change:
In #1519, the subscription_sync_total metric was updated when the
related subscription object was updated. However, the PR had a bug,
in which the map used to store metric related information was not
being updated when the metric was udpated. As a result of which any
update after the first update was not removing the old metric. This
PR fixes that bug.
Motivation for the change:
Reviewer Checklist
/docs