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

add InstallplanApproval (automatic/manual) to OLM metrics #1613

Merged
merged 1 commit into from
Aug 12, 2020
Merged

add InstallplanApproval (automatic/manual) to OLM metrics #1613

merged 1 commit into from
Aug 12, 2020

Conversation

konsotirop
Copy link
Contributor

This PR adds the InstallPlanApproval strategy (automatic/manual) to OLM metrics.

@openshift-ci-robot
Copy link
Collaborator

Hi @konsotirop. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 29, 2020
@konsotirop konsotirop changed the title Installplan add InstallplanApproval (automatic/manual) to OLM metrics Jun 29, 2020
@dinhxuanvu
Copy link
Member

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 30, 2020
@dinhxuanvu dinhxuanvu requested a review from anik120 June 30, 2020 03:05
anik120
anik120 requested changes Jun 30, 2020
View reviewed changes
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

@konsotirop could you rebase your PR too. Thanks!

pkg/metrics/metrics.go Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2020
@konsotirop
Copy link
Contributor Author

/retest

anik120
anik120 requested changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@anik120 anik120 left a comment

Choose a reason for hiding this comment

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

@konsotirop thanks for addressing the feedback. Left a couple of comments. Thanks

test/e2e/metrics_e2e_test.go Outdated Show resolved Hide resolved
@konsotirop
Copy link
Contributor Author

/retest

ecordell
ecordell reviewed Jul 7, 2020
View reviewed changes
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Outdated Show resolved Hide resolved
@konsotirop
Copy link
Contributor Author

/retest

1 similar comment
@konsotirop
Copy link
Contributor Author

/retest

awgreene
awgreene requested changes Jul 9, 2020
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

Nice work @konsotirop - just one or two questions.

pkg/metrics/metrics.go Outdated Show resolved Hide resolved
pkg/metrics/metrics.go Show resolved Hide resolved
@exdx
Copy link
Member

exdx commented Jul 22, 2020

Could you squash all your commits down to just one? Looks great btw.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@konsotirop
Copy link
Contributor Author

Could you squash all your commits down to just one? Looks great btw.
/lgtm

@exdx thanks, i squashed the commits

@exdx
Copy link
Member

exdx commented Jul 23, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@Bowenislandsong
Copy link
Member

This PR failed tests for 1 times with 1 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits.

totaltestcount: 1
flaketestcount: 1
skippedtestcount: 4
flaketests:

  • classname: End-to-end
    name: 'Subscription creation manual approval'
    counts: 1
    details:

    • count: 1
      error: |4-

      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go:178

	Error Trace:	subscription_e2e_test.go:185
	            				runner.go:113
	            				runner.go:64
	            				it_node.go:26
	            				spec.go:215
	            				spec.go:138
	            				spec_runner.go:200
	            				spec_runner.go:170
	            				spec_runner.go:66
	            				suite.go:62
	            				ginkgo_dsl.go:226
	            				ginkgo_dsl.go:214
	            				e2e_test.go:54
	Error:      	Received unexpected error:
	            	E2E bug detected: configmaps "mock-ocs" already exists

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/vendor/github.com/stretchr/testify/require/require.go:1005
    meandurationsec: 0.067491
    skippedtests:
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should have created the expected
    prometheus objects    '
    counts: 1
    details: []
    meandurationsec: 2.141728
  • classname: End-to-end
    name: 'Subscription updates existing install plan'
    counts: 1
    details: []
    meandurationsec: 0.252876
  • classname: End-to-end
    name: 'Catalog image update'
    counts: 1
    details: []
    meandurationsec: 0.015089
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should install the operator
    successfully    '
    counts: 1
    details: []
    meandurationsec: 2.141835

@konsotirop
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2020
@Bowenislandsong
Copy link
Member

This PR failed tests for 1 times with 1 individual failed tests and 4 skipped tests. A test is considered flaky if failed on multiple commits.

totaltestcount: 1
flaketestcount: 1
skippedtestcount: 4
flaketests:

  • classname: End-to-end
    name: 'Install Plan with CRD schema change Test all existing versions are present,
    different (backwards incompatible) schema    '
    counts: 1
    details:

    • count: 1
      error: |4-

      /home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/vendor/github.com/onsi/ginkgo/extensions/table/table_entry.go:43

	Error Trace:	installplan_e2e_test.go:775
	            				value.go:460
	            				value.go:321
	            				table_entry.go:37
	            				runner.go:113
	            				runner.go:64
	            				it_node.go:26
	            				spec.go:215
	            				spec.go:138
	            				spec_runner.go:200
	            				spec_runner.go:170
	            				spec_runner.go:66
	            				suite.go:62
	            				ginkgo_dsl.go:226
	            				ginkgo_dsl.go:214
	            				e2e_test.go:54
	Error:      	Received unexpected error:
	            	etcdserver: request timed out

/home/runner/work/operator-lifecycle-manager/operator-lifecycle-manager/vendor/github.com/stretchr/testify/require/require.go:1005
    meandurationsec: 20.60687
    skippedtests:
  • classname: End-to-end
    name: 'Subscription updates existing install plan'
    counts: 1
    details: []
    meandurationsec: 0.671212
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should have created the expected
    prometheus objects    '
    counts: 1
    details: []
    meandurationsec: 3.768332
  • classname: End-to-end
    name: 'Catalog image update'
    counts: 1
    details: []
    meandurationsec: 0.083156
  • classname: End-to-end
    name: 'Subscriptions create required objects from Catalogs Given a Namespace
    when a CatalogSource is created with a bundle that contains prometheus objects
    creating a subscription using the CatalogSource should install the operator
    successfully    '
    counts: 1
    details: []
    meandurationsec: 2.133973

@konsotirop
Copy link
Contributor Author

/retest

2 similar comments
@konsotirop
Copy link
Contributor Author

/retest

@konsotirop
Copy link
Contributor Author

/retest

njhale
njhale approved these changes Aug 4, 2020
View reviewed changes
Copy link
Member

@njhale njhale left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2020
@konsotirop
Copy link
Contributor Author

/retest

1 similar comment
@konsotirop
Copy link
Contributor Author

/retest

awgreene
awgreene reviewed Aug 12, 2020
View reviewed changes
pkg/metrics/metrics.go
@@ -208,8 +210,8 @@ func RegisterCatalog() {
prometheus.MustRegister(dependencyResolutionSummary)
}

func CounterForSubscription(name, installedCSV, channelName, packageName string) prometheus.Counter {
return SubscriptionSyncCount.WithLabelValues(name, installedCSV, channelName, packageName)
func CounterForSubscription(name, installedCSV, channelName, packageName, planApprovalStrategy string) prometheus.Counter {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func CounterForSubscription(name, installedCSV, channelName, packageName, planApprovalStrategy string) prometheus.Counter {
func CounterForSubscription(name, installedCSV, channelName, packageName, installplanApprovalStrategy string) prometheus.Counter {

awgreene
awgreene approved these changes Aug 12, 2020
View reviewed changes
Copy link
Member

@awgreene awgreene left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, konsotirop, njhale

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 1fdd347 into operator-framework:master Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ecordell ecordell ecordell left review comments

@anik120 anik120 anik120 requested changes

@awgreene awgreene awgreene approved these changes

@njhale njhale njhale approved these changes

@dinhxuanvu dinhxuanvu Awaiting requested review from dinhxuanvu

@exdx exdx Awaiting requested review from exdx

Assignees

@awgreene awgreene

@exdx exdx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.