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(java): replace callbacks by CompletableFuture APIC-421 #452

Merged
merged 17 commits into from
May 10, 2022

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Apr 28, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-421

Use CompletableFuture to make the async function usable, which are widely used by customers.
Thanks to @aallam this allows to remove the custom response for EchoRequester and simply pass the value by a different route, thus decoupling the test and the client (very good 😄 ).

Changes included:

  • Fix deprecation warnings
  • Remove lots of mustache in favor of hand generated files
  • Use CompletableFuture
  • Simplify the clients by removing methods
  • Split the logic of Http and Echo requesters
  • Add Insights playground

🧪 Test

@millotp millotp self-assigned this Apr 28, 2022
@netlify
Copy link

netlify bot commented Apr 28, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 59df1c7
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/627a7b268ae52e00089236b6

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 28, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

openapitools.json Outdated Show resolved Hide resolved
@millotp millotp changed the base branch from main to chore/ci-cache-simple May 3, 2022 12:03
@millotp millotp force-pushed the feat/java-async branch 2 times, most recently from d9764ec to d8344b0 Compare May 9, 2022 14:42
@millotp millotp changed the base branch from chore/ci-cache-simple to main May 9, 2022 14:43
@millotp millotp requested review from shortcuts, aallam, a team and damcou and removed request for a team May 10, 2022 12:59
@millotp millotp marked this pull request as ready for review May 10, 2022 13:01
shortcuts
shortcuts previously approved these changes May 10, 2022
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Nothing to say from my Java noob eyes, nice cleanup!

Comment on lines 228 to 230
run: |
rm -rf ${{ matrix.client.testsOutputPath }}/client || true
rm -rf ${{ matrix.client.testsOutputPath }}/methods || true
Copy link
Member

@shortcuts shortcuts May 10, 2022

Choose a reason for hiding this comment

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

I'd prefer paths being defined in createMatrix to avoid extra logic here, but up to you

shortcuts
shortcuts previously approved these changes May 10, 2022
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

perfect!!!!!!!

@@ -235,7 +236,7 @@ jobs:
if: ${{ needs.setup.outputs.RUN_CTS == 'true' }}
run: |
git --no-pager diff
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
git --no-pager diff
git --no-pager diff ${{ matrix.client.testsOutputPathToClean }}

would make the error less confusing

@millotp millotp requested a review from shortcuts May 10, 2022 14:41
Comment on lines 231 to 232
exit $(git diff --name-only --diff-filter=d ${{ matrix.client.testsOutputPath }} | wc -l)

Copy link
Member

Choose a reason for hiding this comment

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

If we remove this now, there's no way to ensure pushed tests are the ones that runs, I'd rather only remove this when we make the CI push the tests

@millotp millotp enabled auto-merge (squash) May 10, 2022 14:56
@millotp millotp requested a review from shortcuts May 10, 2022 14:56
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

:yes:

@millotp millotp merged commit 961a4b5 into main May 10, 2022
@millotp millotp deleted the feat/java-async branch May 10, 2022 14:59
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