Exclude id from NetworkImpl.NetworkImplBuilder (testcontainers#8125)

[quincy]

This seemed like an easy issue to get started contributing. I'm a big fan of the project and have been evangelizing it at my company.

[rnorth]

We had some issues with the quay.io docker registry, which caused the build to fail. Will update the branch from master to include #2805.

[quincy]

Thanks @rnorth, I saw quay was having problems again.

[rnorth]

Looks like we have a check failure in the :testcontainers:japicmp task 🤔
We may have a binary backwards compatibility problem to think about here...

[bsideup]

@rnorth we definitely do, because the constructor was public :) Let's make it backward compatible and deprecate first

[quincy]

Assuming you want something like this?

@Deprecated
public NetworkImpl() {}

[bsideup]

@quincy no, the other one ("all args constructor"), with the id

[quincy]

Ah, right, because @Builder would need to create an all-args constructor in order for it to do its job.

[quincy]

That doesn't appear to have fixed the issue. It's still complaining about the removed setter.

[quincy]

Okay, I've pushed a new commit which delomboks the existing builder, and deprecates the id method directly. After this has been released we can go back in and remove the deprecated stuff and throw the @Builder annotation back on.

If you guys know a better way to deal with this through lombok directly, please clue me in. I did not find anything in their documentation which looked promising.

[bsideup]

@quincy I wonder if adding @Deprecated on a field makes it deprecated in the builder, too. Could you please check?

[quincy]

@bsideup, yes, marking the field deprecated makes it deprecated in the builder. I believe you are suggesting we could mark the id field deprecated for now and release. Then after the release we can come back and implement my original change to have the builder without the id, and at that point we could remove the deprecated annotation from id.

Let me know if that sounds good, or if you have something else in mind, and I can get that change pushed.

[bsideup]

@quincy yes, this definitely sounds good 👍

[quincy]

@bsideup, @rnorth,

Was there anything else you guys wanted me to do on this branch at the moment?

[bsideup]

@quincy thanks for the ping. I guess we should also override getId and not mark it as deprecated, because the getter is definitely not :)

[quincy]

@bsideup, the getter is already overridden.

        @Override
        public synchronized String getId() {
            if (initialized.compareAndSet(false, true)) {
                id = create();
            }

            return id;
        }

I am not really familiar with lombok so I am not sure how to solve that. Any suggestions?

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

[quincy]

I got quite a bit more busy so I kind of lost track of this, but I'm still interested in finishing this PR. I'm not sure what the best way forward is though, are there any suggestions about how to deal with the getter being marked deprecated?

[rnorth]

I think the PR is having the desired effect:

I'm going to approve but not merge; @bsideup if you think this is missing something still please say.

[bsideup]

@quincy @rnorth looks good! 💯

[quincy]

Thanks @bsideup , @rnorth !