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

I'd like to contribute a port of the rollout status code... have a few questions before I proceed #6243

Open
mikebell90 opened this issue Aug 10, 2024 · 8 comments
Labels

Comments

@mikebell90
Copy link

mikebell90 commented Aug 10, 2024

Is your enhancement related to a problem? Please describe

https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/rollout_status.go

Describe the solution you'd like

I have ported this and the basic tests. Please understand this is a partial port in that I'm not trying to port the watcher or other such logic, just the "instantaneous" status. But it's a grea building block.

So my questions:

  • Is this of use to folks
  • Which module and package should it go - looks like either client or client-api. I would normally say "client" but the utils that seem closest seem to be in utils.
  • The original structure includes an Error returned which doesn't have good java equivalent. Ok to return an enum?

Describe alternatives you've considered

No response

Additional context

No response

@mikebell90
Copy link
Author

My solution consists of

  • interface RolloutStatus and a factory
  • enum
  • class representing the CurrentRolloutStatus. This is basically the "tuple" message, done, and error (enum).
  • 3 implementations - for deployment, statefulset, and daemonset.

Does this broad structuring sound reasonable at first glance.
I'd rather ask questions before opening a PR, as sometimes I find various OSS projects are very picky, and I have limited time.

@mikebell90
Copy link
Author

Some of this is previously discussed in #2904

@mikebell90
Copy link
Author

Also

  • Any guidelines as to tests etc. I've chosen to pattern them after the ported tests from Go, which admittedly makes them look a tiny bit "weird" - Loaded array of "test" classes representing inputs and expectations
  • Any guidelines as to code format.
  • In general I've deviated to generally following ugly go-port for consistency but wrapping in null checks etc. But I certainly haven't made them perfect java idiom - deliberately so for comparison, but ...

@manusa
Copy link
Member

manusa commented Aug 12, 2024

Does this broad structuring sound reasonable at first glance.

Haven't digged much on the Go implementation, but your proposal seems reasonable.

Maybe the interface should be later extended by RollableScalableResource (I'm assuming this functionality is related to these objects/resources).
This also relates to your 3 implementations - for deployment, statefulset, and daemonset, so the implementation should be done for all RollableScalableResource implementations.

Any guidelines as to tests etc.

Check the tests we have for rollable scalable resources in general.

Also, checking the Go implementation, you might want to expose the logic in a package-private way so that you can create unit tests to verify all possible conditions.
A static helper might simplify things very much.

Any guidelines as to code format

Formatting is automatic upon compilation. You can also apply format running mvn spotless:apply -Pitests.

Not that you'll also need to add license headers for any new class or file you create mvn -N license:format

/cc @shawkins

@mikebell90
Copy link
Author

Maybe the interface should be later extended by RollableScalableResource (I'm assuming this functionality is related to these objects/resources).

Well Daemonsets haven't been implemented for this on the fabric8 side, and I don't really have time to add that too.

@mikebell90
Copy link
Author

Thanks, I've requested permission from my company, and once that's resolved will open a PR linked to this issue.

Copy link

stale bot commented Nov 16, 2024

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

@stale stale bot added the status/stale label Nov 16, 2024
@mikebell90
Copy link
Author

So I finally got approval a week or two. Trying to find time to repackage the code. This will probably be in december as abut to have a heart operation

@stale stale bot removed the status/stale label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants