-
Notifications
You must be signed in to change notification settings - Fork 83
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
Extract image SHAs from status.containerStatuses as well as spec.containers #395
Extract image SHAs from status.containerStatuses as well as spec.containers #395
Conversation
Skipping CI for Draft Pull Request. |
Yes, sidecars will show up and be counted in the deploytime metric. This happens in both the spec and the status. However, this is a tricky problem to solve overall-- a commit in a repo could change some config-as-code that affects the sidecar, meaning the metric would be valid. In any case, since this is an existing issue, that's separate from this PR. |
Bump. Any updates on this? |
@etsauer the update is that I had a nice vacation and am back to working on this today. All that's left is to add / check the mocked API for testing this, and investigate the unit test failure that CI turned up. |
@KevinMGranger 🤣 welcome back! |
See dora-metrics#395 (comment) for details.
I forgot that we only have mocks for committime for now. Mocking this can come later-- the unit test should be sufficient. |
Hmm, looks like our python version pinning is complaining about the new version. @mpryc, is there a reason we restricted the patch version of the python version? |
@KevinMGranger there is not, I think it's safe to include the removal of patch in this PR. |
See dora-metrics#395 (comment) for details.
83f41c1
to
6b3e734
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: etsauer, KevinMGranger 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 |
Behavior Changes
The deploytime exporter will now check a pod's
status.containerStatuses
for image IDs as well.Linked Issues?
resolves #330
Testing Instructions
I'm planning on adding a simple mock for our deploytime exporter as well, which will have some images that don't have the ID in their spec.
@redhat-cop/mdt
Remaining Work
pod.status.containerStatuses
might list sidecars from MutatingAdmissionControllers (e.g. from Istio / Service Mesh), which we wouldn't want to count. It's also possible that those are already included inspec.containers
, which would be a separate issue.[ ] I need to add the mock as mentioned above.