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

feat: add ReportMetrics to azureidentity.go #1870

Closed
wants to merge 1 commit into from

Conversation

SpongeBob0318
Copy link

@SpongeBob0318 SpongeBob0318 commented Oct 15, 2024

Description

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Fixes #1868

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Checklist:

  • Does the affected code have corresponding tests?
  • Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have appropriate license header?

Post Merge Requirements

  • MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

Copy link

codecov bot commented Oct 16, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...kg/common/oras/authprovider/azure/azureidentity.go 0.00% 4 Missing ⚠️
Files with missing lines Coverage Δ
...kg/common/oras/authprovider/azure/azureidentity.go 21.50% <0.00%> (-3.14%) ⬇️

... and 130 files with indirect coverage changes

@binbin-li
Copy link
Collaborator

@SpongeBob0318 thanks for making the PR! Could you fix the lint error and update the title to follow conventional commit spec?

  pkg/common/oras/authprovider/azure/azureidentity.go:22: File is not `goimports`-ed (goimports)
  	"github.com/ratify-project/ratify/pkg/metrics"

@shahramk64
Copy link
Contributor

@SpongeBob0318 Codecov fails because we now have a requirement that new PRs should have %80 coverage for unit testing. You need to define unit tests and make sure they pass on your PR.
Have a look at this PR: #1829
Here you can see how an interface is defined to be able to utilise dependency injection in your unit tests.

@SpongeBob0318
Copy link
Author

@SpongeBob0318 Codecov fails because we now have a requirement that new PRs should have %80 coverage for unit testing. You need to define unit tests and make sure they pass on your PR. Have a look at this PR: #1829 Here you can see how an interface is defined to be able to utilise dependency injection in your unit tests.

great , i'll try

@akashsinghal akashsinghal changed the title Add ReportMetrics to azureidentity.go feat: add ReportMetrics to azureidentity.go Oct 17, 2024
@junczhu
Copy link
Collaborator

junczhu commented Oct 22, 2024

Updated issue link

@shahramk64
Copy link
Contributor

@SpongeBob0318 Hey, are you still interested in continuing the work on this PR?

@susanshi
Copy link
Collaborator

susanshi commented Dec 3, 2024

This is out of date with Dev branch, and conflicts needs to be resolved. Please reopen PR once we have capacity.

@susanshi susanshi closed this Dec 3, 2024
@SpongeBob0318
Copy link
Author

SpongeBob0318 commented Dec 3, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ReportMetrics to azureidentity.go
5 participants