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

Uptake pekko jars, use pekko names in packages and configs #23

Merged
merged 4 commits into from Feb 27, 2023

Conversation

pjfanning
Copy link
Contributor

@pjfanning pjfanning commented Feb 24, 2023

@pjfanning pjfanning self-assigned this Feb 24, 2023
@pjfanning pjfanning marked this pull request as draft February 24, 2023 12:15
@pjfanning pjfanning marked this pull request as ready for review February 26, 2023 00:35
@pjfanning pjfanning changed the title [DRAFT] uptake pekko jars, use pekko names in packages and configs uptake pekko jars, use pekko names in packages and configs Feb 26, 2023
@jrudolph
Copy link
Contributor

jrudolph commented Feb 27, 2023

Great stuff!

  • temporarily added akka-http-cors in this repo because that lib does not yet support pekko - but soon will

Isn't that a blocker for merging this PR for now? Feels wrong to add third-party code as part of such a big commit just to get everything working right now.

How about we just disable cors support for now to make this PR mergable (it's only used in four lines in the code) and then decide afterwards if we want to temporarily enable it again by importing the third-party project in its own PR or wait until the project itself has released something?

@jrudolph
Copy link
Contributor

  • temporarily added akka-http-cors in this repo because that lib does not yet support pekko - but soon will

Isn't that a blocker for merging this PR for now? Feels wrong to add third-party code as part of such a big commit just to get everything working right now.

If we would really want to add it to this project, we would have change its package name to avoid future problems for users which use both pekko-http-cors and pekko-grpc.

@pjfanning
Copy link
Contributor Author

I guess I had the pekko version of akka-http-cors ready so it seemed easiest to use it but I can look at disabling cors support instead.

@mdedetrich
Copy link
Contributor

mdedetrich commented Feb 27, 2023

Another option would be to wait a couple of weeks, the owner of akka-http-cors did say they are going to migrate over to pekko and since the akka-http-cors is pretty trivial there shouldn't be too much of a risk of it taking too long.

@pjfanning
Copy link
Contributor Author

The cors code in pekko-grpc does look like it is non-trivial to rip out (and later add back) - so we might be best off waiting

@pjfanning pjfanning marked this pull request as draft February 27, 2023 14:37
@pjfanning pjfanning changed the title uptake pekko jars, use pekko names in packages and configs [DRAFT] uptake pekko jars, use pekko names in packages and configs Feb 27, 2023
@jrudolph
Copy link
Contributor

The cors code in pekko-grpc does look like it is non-trivial to rip out (and later add back) - so we might be best off waiting

Was easy enough, I added it here: pjfanning#1. This might break something but it's only used in grpc-web which is less used than regular grpc support. I don't mind waiting for cors support, on the other hand, getting a snapshot version out which mostly works besides grpc-web/cors support might still be interesting.

@pjfanning pjfanning changed the title [DRAFT] uptake pekko jars, use pekko names in packages and configs Uptake pekko jars, use pekko names in packages and configs Feb 27, 2023
@pjfanning pjfanning marked this pull request as ready for review February 27, 2023 16:14
@pjfanning
Copy link
Contributor Author

I merged @jrudolph change. I'm happy to have this merged if the consensus is that we can look at adding back the CORS support in a few weeks.

@mdedetrich
Copy link
Contributor

Fine with me, just make an issue for it so we don't forget

@pjfanning
Copy link
Contributor Author

pjfanning commented Feb 27, 2023

There is #22

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

So I did a more comprehensive look at the PR and I noticed that @apidoc was being removed, is there a reason why?

If its because it didn't compile, you usually need to update the Paradox prefix, i.e. setting ApidocPlugin.autoImport.apidocRootPackage to org.apache.pekko should fix the problem.

docs/src/main/paradox/client/details.md Outdated Show resolved Hide resolved
@pjfanning
Copy link
Contributor Author

I've tried out the apidoc changes but CI is struggling due to Jabba connectivity issues

@mdedetrich
Copy link
Contributor

mdedetrich commented Feb 27, 2023

Hmm, the jabba issues seem completely unrelated, jabba is used in setup-scala github action just to download JDK. Let me see

@mdedetrich
Copy link
Contributor

mdedetrich commented Feb 27, 2023

Yeah so everything related to JDK 11 is failing because of

      - name: Set up JDK 11
        uses: olafurpg/setup-scala@v13
        with:
          java-version: adopt@1.11

If we change this to setup-java instead of setup-scala (like we did with pekko) this should solve the problem, i.e.

      - name: Setup Java 11
        uses: actions/setup-java@v3
        with:
          distribution: temurin
          java-version: 11

@pjfanning
Copy link
Contributor Author

Jabba will probably start working again. I can do a separate PR for setup-java switch. I can do that now.

@mdedetrich
Copy link
Contributor

Jabba will probably start working again. I can do a separate PR for setup-java switch. I can do that now.

I think it makes sense to do it now, its a trivial PR and needs to be done anyways (setup-scala as we can see has problems).

pjfanning and others added 3 commits February 27, 2023 23:01
scalafmt and disable header check

sbt-pekko-grpc

doc build

rename more classes

proto package names

pekko configs

Update VersionGenerator.scala

pekko-http config

pekko-http config

benchmark-java compile

Delete release-drafter.yml

doc issues

fix doc build

temporarily remove `@apidoc` refs that are broken

try to fix build resolve issue

try to fix build

try to fix plugin tests

more akka refs

try to fix sbt tests

more sbt plugin test changes

test issue

try again to fix sbt tests

try to fix more tests

scalafmt

more akka refs

more akka refs

Update PekkoGrpcPlugin.scala

Update PekkoGrpcPlugin.groovy

Update PekkoGrpcPlugin.groovy

Update PekkoGrpcPlugin.groovy

fix names of artifacts

add scala-library

revert speculative change

name issue

Update build.gradle

Update build.gradle

revert unwanted changes
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

Okay I am going to approve this, tests are passing. I noticed some more cases where we are still using Akka in class/object names so feel free to update those before merging.

As mentioned before the only other thing that needs to be changed is to make the import style consistent with the Pekko akka projects, I can go ahead and do this after the PR is merged.

scalafmt
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.

None yet

3 participants