Skip to content

Conversation

@yue9944882
Copy link
Member

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2023
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 4, 2023
@yue9944882 yue9944882 force-pushed the rename-legacy-modules branch from d6fa884 to fd2c47c Compare December 4, 2023 18:56
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 4, 2023
@yue9944882 yue9944882 changed the title Rename {client-java-api,client-java-fluent} to *-legacy Rename {client-java-api,client-java-fluent,fluent-gen} to *-legacy Dec 4, 2023
@brendandburns
Copy link
Contributor

Do we really need to move all of the fluent files? I think we just need to build for both?

Some more details on this would be useful.

Thanks!

@yue9944882 yue9944882 force-pushed the rename-legacy-modules branch from fd2c47c to af7ad46 Compare December 5, 2023 18:21
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 5, 2023
@yue9944882 yue9944882 changed the title Rename {client-java-api,client-java-fluent,fluent-gen} to *-legacy Rename client-java-api to *-legacy Dec 5, 2023
@yue9944882
Copy link
Member Author

Do we really need to move all of the fluent files? I think we just need to build for both?

hmm. i'm not sure if we can build fluent for both. i removed fluent related changes in PR so we can try generate for both in a follow-up PR

@yue9944882 yue9944882 force-pushed the rename-legacy-modules branch from af7ad46 to 2e59111 Compare December 5, 2023 18:39
Signed-off-by: Min Jin <minkimzz@amazon.com>
@yue9944882 yue9944882 force-pushed the rename-legacy-modules branch from 2e59111 to 3cba1c3 Compare December 5, 2023 18:43
@brendandburns
Copy link
Contributor

Sorry, I'm thinking about this some more. Do you think it would be easier to maintain this as a branch in git, rather than duplicating the code on master?

@yue9944882
Copy link
Member Author

Sorry, I'm thinking about this some more. Do you think it would be easier to maintain this as a branch in git, rather than duplicating the code on master?

that's definitely viable but i'm slight worried if it's as convenient to maintain as a legacy module. additionally, putting it into a separate branch will sort of double the effort for code generation for every kubernetes release because we will need to open (or automate) 2 separate PRs upon every release. do you see any downside of having a legacy module?

@brendandburns
Copy link
Contributor

Aren't we going to have to do double code generation runs anyway? Or are you suggesting that we update the code generation script to just run twice?

I personally would vote for the branch approach, it's worked reasonably well in the Javascript client. We setup dependabot etc to send PRs to both repos.

I'm sort of ok with the copy everything approach, but I worry it's going to be a lot of duplicated code that is going to make the repository messy and also make all of the CI/CD runs twice as long.

Also, since presumably we're going to eventually stop supporting the legacy branch except for critical fixes, it feels like we'll just eventually have to delete all the legacy code which will make it harder to use the legacy library.

It also feels simpler to implement since we can just branch, with the current state that works with java8, and then convert main to be java11.

But ultimately, if you feel strongly, since you are doing the work, I defer to your preference.

@yue9944882
Copy link
Member Author

@brendandburns gotcha, let's move those to a separate branch then. i will file another PR after closing this!

@yue9944882 yue9944882 closed this Dec 12, 2023
@brendandburns
Copy link
Contributor

Sounds good. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants