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 factory methods to help with SSA #6013

Merged
merged 2 commits into from
May 28, 2024
Merged

feat: add factory methods to help with SSA #6013

merged 2 commits into from
May 28, 2024

Conversation

metacosm
Copy link
Collaborator

@metacosm metacosm commented May 16, 2024

Description

Fixes #6012

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@metacosm metacosm requested a review from manusa as a code owner May 16, 2024 14:19
@metacosm metacosm self-assigned this May 16, 2024
@metacosm metacosm force-pushed the new-instance branch 3 times, most recently from 02f3589 to 47a508b Compare May 22, 2024 14:44
@manusa manusa requested a review from shawkins May 23, 2024 12:18
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

Looks good, just two more thoughts.

* @return a new ObjectMetaBuilder instance initialized with the name and namespace (if the resource it is called on is a
* namespaced one) of the specified ObjectMeta
*/
default ObjectMetaBuilder initMetadataBuilderNameAndNamespaceFrom(ObjectMeta original) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this one static - it does not need access to the current instance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense. I originally hoped that I could set the metadata on the current instance as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

static ObjectMetaBuilder initMetadataBuilderNameAndNamespaceFrom(ObjectMeta original)

gives you the usage:

new SomethingBuilder().withMetadata(HasMetadata.initMetadataBuilderNameAndNamespaceFrom(original)....build())...

An alternative which doesn't quite set the metadata, but allows you to do so more fluently:

static <A extends ObjectMetaFluent<A>> A withNameAndNamespace(A value, HasMetadata original) {

gives you the usage:

HasMetadata.withNameAndNamespace(new SomethingBuilder().withNewMetadata(), original)...endMetadata()...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it does access the current instance to check if it is Namespaced or not to decide whether to use and set the namespace…

Copy link
Contributor

Choose a reason for hiding this comment

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

@metacosm I think I was assuming based upon your other comment though it could instead be:

static ObjectMetaBuilder initMetadataBuilderNameAndNamespaceFrom(HasMetadata original)

But didn't make that change in the signature shown above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotcha. Sorry for being slow, it's been a long day. 😓

@metacosm metacosm force-pushed the new-instance branch 3 times, most recently from c4607e7 to baac39f Compare May 23, 2024 20:42
@manusa manusa requested a review from shawkins May 28, 2024 10:00
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

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

LGTM

@manusa manusa added this to the 6.13.0 milestone May 28, 2024 — with automated-tasks
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

Copy link

@manusa manusa merged commit e0647d7 into main May 28, 2024
21 checks passed
@manusa manusa deleted the new-instance branch May 28, 2024 13:18
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 utility method to easily create a named/namespaced empty copy of a given resource
5 participants