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

publish finch artifacts using github actions #1461

Merged
merged 12 commits into from
Apr 8, 2022

Conversation

arron-green
Copy link
Contributor

worked with @rpless to prepare deploying artifacts with github actions.

still needs a github actions file to trigger workflow

@arron-green arron-green mentioned this pull request Apr 7, 2022
@joroKr21
Copy link
Collaborator

joroKr21 commented Apr 7, 2022

I think we need to revert #1459 and #1460 first

@arron-green
Copy link
Contributor Author

this is also failing due to scalafix errors. running sbt validate shows the errors

fixed locally with sbt scalafixAll

@joroKr21
Copy link
Collaborator

joroKr21 commented Apr 7, 2022

A no wait, you brought back the build here 🤔

@arron-green
Copy link
Contributor Author

got past the scalafix issue and now validate is failing due to

[error] (refined / Compile / scalafmtCheck) org.scalafmt.sbt.ScalafmtSbtReporter$ScalafmtSbtError: scalafmt: Invalid config: Default dialect is deprecated; use explicit: [sbt0137,sbt1,scala211,scala212,scala212source3,scala213,scala213source3,scala3]
[error] Also see https://scalameta.org/scalafmt/docs/configuration.html#scala-dialects"

@rpless
Copy link
Collaborator

rpless commented Apr 7, 2022

The config is probably out of date. Dialect is a new thing. https://scalameta.org/scalafmt/docs/configuration.html#scala-dialects. We probably want scala213.

@joroKr21
Copy link
Collaborator

joroKr21 commented Apr 7, 2022

You need to add runner.dialect = scala213 - it should work also fine with Scala 2.12

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
@arron-green
Copy link
Contributor Author

left to do

Copy link
Collaborator

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

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

A few more nits to hammer.
Another question - I think once merged this it would release 0.33.0 - is this desired?

examples/src/main/scala/io/finch/iteratee/Main.scala Outdated Show resolved Hide resolved
examples/src/main/scala/io/finch/wrk/Finagle.scala Outdated Show resolved Hide resolved
examples/src/main/scala/io/finch/wrk/Finagle.scala Outdated Show resolved Hide resolved
examples/src/main/scala/io/finch/wrk/Finch.scala Outdated Show resolved Hide resolved
examples/src/main/scala/io/finch/wrk/Finch.scala Outdated Show resolved Hide resolved
Comment on lines +15 to +18
fail-fast: false
matrix:
os: [ubuntu-latest]
java: ['adopt@1.11']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just delete those and inline os and java version

@@ -8,8 +8,7 @@ import io.finch.internal.HttpContent

trait Decoders {

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any chance we can undo these comment changes so it's easier to review?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe open a separate PR with code style changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. it was just the output of running scalafmt and scalafix

Copy link
Collaborator

@vkostyukov vkostyukov left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me! Thank you so much, @arron-green and @rpless and @joroKr21 for these changes!

@rpless
Copy link
Collaborator

rpless commented Apr 8, 2022

This PR looks good to me as well, however there is one minor problem that we've uncovered. I was able to confirm that the encryption password works for the files on a mac, however when I tried on an ubuntu machine it failed with the following:

| *** WARNING : deprecated key derivation used.
| Using -iter or -pbkdf2 would be better.
| bad decrypt
| 281473334143456:error:06065064:digital envelope routines:EVP_DecryptFinal_ex:bad decrypt:../crypto/evp/evp_enc.c:615:

A security engineer at work pointed me to this and said the problem was most likely that the files were originally encrypted with libressl and that apparently doesn't work with versions of ssl that ship with ubuntu.
I've also used https://github.com/nektos/act to test the Github Action locally and the files fail to decrypt with the same error in the container that github actions runs. @arron-green did some digging and found this workaround which involves re-encrypting the files.
So here is what I propose:

  • We revert the release commit
  • We then merge this PR
  • We then merge Fix formatting (scalafmt & scalafix) #1465
  • I'll push a commit with those files re-encrypted such that the openssl in the github actions container can decrypt them
  • We attempt the release again

If folks are good with this plan let me and I can get it started.

@arron-green
Copy link
Contributor Author

@rpless sounds good to me. i'll update the build script to include the openssl options to include -salt -pbkdf2. i can comment the rest of the build to only include the decrypt portion to test with github actions

@arron-green
Copy link
Contributor Author

i realized after i pushed my last update that the openssl section would only run on merge. moved to the top to test out that portion

@arron-green
Copy link
Contributor Author

@rpless script is ready for the updated files

@rpless
Copy link
Collaborator

rpless commented Apr 8, 2022

Ok I'll get started on that process I described.

Copy link
Collaborator

@joroKr21 joroKr21 left a comment

Choose a reason for hiding this comment

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

Now you just need to update the branch I think

@rpless rpless merged commit 94dfab0 into finagle:master Apr 8, 2022
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.

4 participants