Skip to content

Conversation

geanpalacios
Copy link

This adds support for readinessProbe on containers.
I have a use case that would benefit from having this.

Since livenessProbe support is already a thing, I took from #158 to add readinessProbe related changes.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@geanpalacios geanpalacios requested a review from a team as a code owner February 16, 2023 03:20
@geanpalacios geanpalacios force-pushed the add-readiness-probe-support branch from 0ab2f8b to f093342 Compare February 16, 2023 03:44
Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

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

Are you aware you can already do this with the yaml syntax? The container liveness probe was added 6 years ago and it wasn't available then.

Keeping track of all new fields in Kubernetes is a never-ending game, I don't believe there is much value in adding 1-to-1 mappings to the plugin.

<periodSeconds>0</periodSeconds>
<successThreshold>0</successThreshold>
</livenessProbe>
<readinessProbe>
Copy link
Member

Choose a reason for hiding this comment

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

Please revert all these test resources hunks. They represent old configuration, they must not be modified in order to let tests pass.

/**
* Created by geanpalacios on 15/02/23.
*/
public class ContainerReadinessProbe extends AbstractDescribableImpl<ContainerReadinessProbe> implements Serializable {
Copy link
Member

Choose a reason for hiding this comment

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

Looks identical to ContainerLivenessProbe.

I would suggest to rename ContainerLivenessProbe to ContainerProbe, and add a compatibility alias as documented in http://www.jenkins.io/doc/developer/persistence/backward-compatibility/#rename-a-class

@geanpalacios
Copy link
Author

Are you aware you can already do this with the yaml syntax?

I understand, we provision our containers on different workflows using a standard library on a need basis and not all pods are created the same or have the same, lets say, roles configuration, volumes and such for example.

Having the possibility to add a readinessProbe whenever needed will allow for better container configuration/provisioning while keeping it simple enough to be maintainable

@kh0ma
Copy link

kh0ma commented Oct 17, 2023

Is this still relevant? I also prefer to use a readiness probe at the container level rather than yaml. 👍 for this feature.

@Vlatombe
Copy link
Member

Is this still relevant?

Probably, though there are standing comments that need to be addressed.

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