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

test: fix intermittent operator metrics test failure #12737

Merged
merged 4 commits into from
Mar 5, 2024
Merged

test: fix intermittent operator metrics test failure #12737

merged 4 commits into from
Mar 5, 2024

Conversation

blkperl
Copy link
Contributor

@blkperl blkperl commented Mar 4, 2024 •
edited

Motivation

Fixes intermittent unit test failures

Modifications

Verification

@blkperl
test: fix intermittent operator metrics test failure
a59672a 
Signed-off-by: william.vanhevelingen <william.vanhevelingen@acquia.com>
@blkperl blkperl mentioned this pull request Mar 4, 2024
Closed
@agilgur5
Copy link
Member

agilgur5 commented Mar 5, 2024 •
edited

Follow-up to #12702 and #12730, for reference.
And upstream issue that is mentioned in the in-line comment: prometheus/client_model#83

@agilgur5 agilgur5 added area/build Build or GithubAction/CI issues area/upstream This is an issue with an upstream dependency, not Argo itself labels Mar 5, 2024
terrytangyuan
terrytangyuan approved these changes Mar 5, 2024
View reviewed changes
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thank you!

@blkperl
test: apply feedback
63d37dd 
Signed-off-by: william.vanhevelingen <william.vanhevelingen@acquia.com>
@blkperl
chore: rebuild pipeline
7676250 
Signed-off-by: william.vanhevelingen <william.vanhevelingen@acquia.com>
@SuperQ
Copy link

SuperQ commented Mar 5, 2024

Due to the intentionally breaking changes in upstream protobuf, I would recommend refactoring your tests to render to proto and use the proto compare tooling.

@blkperl
chore: rebuild pipeline
5184505 
Signed-off-by: william.vanhevelingen <william.vanhevelingen@acquia.com>
@agilgur5 agilgur5 merged commit e00abd1 into argoproj:main Mar 5, 2024
27 checks passed
@blkperl blkperl deleted the operator_metric_test_failure branch March 5, 2024 19:01
@agilgur5
Copy link
Member

agilgur5 commented Mar 5, 2024 •
edited

Due to the intentionally breaking changes in upstream protobuf, I would recommend refactoring your tests to render to proto and use the proto compare tooling.

See the more detailed comment in the upstream issue: prometheus/client_model#83 (comment), which links to more upstreams prometheus/client_golang#1323 and golang/protobuf#1121

I merged this for now as it was blocking CI intermittently, but indeed, that sounds like we may want to go with that route if it's more accurate. We'd have to analyze if it's worthwhile or if this workaround is "good enough" if we have to add more deps etc to do that

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

@terrytangyuan terrytangyuan terrytangyuan approved these changes

@agilgur5 agilgur5 agilgur5 approved these changes

Assignees
No one assigned
Labels
area/build Build or GithubAction/CI issues area/upstream This is an issue with an upstream dependency, not Argo itself
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@blkperl @agilgur5 @SuperQ @terrytangyuan