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

Verify: add verify function for k8s Jobs #64

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

pit1sIBM
Copy link
Contributor

@pit1sIBM pit1sIBM commented Apr 26, 2024

Related Issue

Supports #65

Related PRs

This PR is not dependent on any other PR

What this PR does / why we need it

Adds a default verify check for k8s Jobs

Special notes for your reviewer

If applicable**

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

What gif most accurately describes how I feel towards this PR?

Example of a gif

Signed-off-by: Zac Pitones <zac.pitones@ibm.com>
Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

@pit1sIBM Thanks for the PR! It occurs to me that there may be several uses for a Job during a rollout:

  1. Start a process that should not block further rollout
  2. Start a process that should block further rollout

This PR clearly tackles (2), but I'm wondering if there's any realistic usecase for (1) that we should consider?

@pit1sIBM
Copy link
Contributor Author

pit1sIBM commented Apr 27, 2024

Thanks @gabe-l-hart, I'm of the mindset that any Job I create is one I expect to complete successfully. If I just want a process to run, I'd probably just create a Pod. So I would prefer Jobs to prevent further rollout since a failure is usually not desired.

I'm still wrapping my head around oper8 in general, but I assume if we wanted to merge this PR and still handle (1), we'd need to override the verify function in our own components to essentially revert the change I'm making here (and only check that the resource exists?)

@HonakerM
Copy link
Collaborator

HonakerM commented Apr 27, 2024

@pit1sIBM thank you for opening this PR! We've needed a default job verification function for a while.

I'm of the mindset that any Job I create is one I expect to complete successfully

I think that makes sense as it's the safest approach.

I'm still wrapping my head around oper8 in general, but I assume if we wanted to merge this PR and still handle (1), we'd need to override the verify function in our own components to essentially revert the change I'm making here (and only check that the resource exists?)

Correct that's what you'd have to do now. However, I've been thinking about this more and we might want to add a verify_function argument to Component.add_resource similar to add_dependency to allow customizing per resource verification without overriding the entire verify. I'll open a PR with what I have in mind.

Copy link
Collaborator

@gabe-l-hart gabe-l-hart left a comment

Choose a reason for hiding this comment

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

Makes sense to me! @HonakerM Thanks for tackling that verify_function addition. With that, I think this is a great default to have.

@gabe-l-hart gabe-l-hart merged commit f38d89a into IBM:main Apr 29, 2024
6 checks passed
@pit1sIBM pit1sIBM deleted the add-job-check branch April 29, 2024 17:43
@gabe-l-hart gabe-l-hart mentioned this pull request May 2, 2024
4 tasks
@pit1sIBM pit1sIBM mentioned this pull request May 24, 2024
3 tasks
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.

3 participants