-
Notifications
You must be signed in to change notification settings - Fork 6
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
Update Scala to 2.13.8 #109
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good, few minor comments! Thanks for taking the time to do this
scheduler/src/test/scala/com/sky/kms/e2e/SchedulerResiliencySpec.scala
Outdated
Show resolved
Hide resolved
- ./prometheus.yml:/etc/prometheus/prometheus.yml | ||
command: | ||
- '--config.file=/etc/prometheus/prometheus.yml' | ||
version: '3.8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need both of these docker-compose files? (this one and the one in the root of the project)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one at the root level is designed to work without the local build, the one inside docker/docker-compose.yml is for local development and can be started with sbt-docker-compose. However I'm not sure of it's use as we don't have any integration tests that use the docker image, so would be open to keeping a single docker-compose file at the root level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would probably opt for keeping this one, its easier to use as it will do the local build for you. We probably should add some e2e tests that use it anyway (in the future).
@@ -26,12 +26,12 @@ object Release { | |||
runTest, | |||
commitReleaseVersion, | |||
tagRelease, | |||
ReleaseStep(releaseStepTask(publish in docker)), | |||
ReleaseStep(releaseStepTask(docker / publish)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried building the image locally like this? I'd be a bit surprised if this works using distroless and sbt-native-packager together. sbt-native-packager tends to create quite complicated shell scripts as entrypoints but distroless images have no shell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. For this PR I've opted to go for https://hub.docker.com/_/eclipse-temurin 's alpine image until we can explore using distroless for this project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you've tested it works right? just wanna make sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scheduler/src/test/scala/com/sky/kms/integration/ScheduleReaderIntSpec.scala
Show resolved
Hide resolved
- ./prometheus.yml:/etc/prometheus/prometheus.yml | ||
command: | ||
- '--config.file=/etc/prometheus/prometheus.yml' | ||
version: '3.8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would probably opt for keeping this one, its easier to use as it will do the local build for you. We probably should add some e2e tests that use it anyway (in the future).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Should remove the bullet in the description that says it closes the distroless issue though.
Related issues
Changes notes
New
runFix
/runFmt
aliases to format and fix code inciBuild
Update
eudev
in the docker image unnecessarily to appease KamonFix