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

Redirect docker buildkit stderr to info #1373

Merged
merged 1 commit into from
Oct 15, 2020
Merged

Redirect docker buildkit stderr to info #1373

merged 1 commit into from
Oct 15, 2020

Conversation

LeonardMeyer
Copy link

@LeonardMeyer LeonardMeyer commented Oct 13, 2020

Fixes #1371
First draft, let me know what you think.

Tested it on my project, it works fine now.

@lightbend-cla-validator

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

@LeonardMeyer
Copy link
Author

Confused about the failing formatting. When I ran scalafmt locally it formatted a lot of files, not just the one I modified...

muuki88
muuki88 previously approved these changes Oct 14, 2020
Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

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

A small nit pick, but otherwise this looks good

Comment on lines 635 to 639
val logger = publishLocalLogger(log)
val ret = sys.process.Process(buildCommand, context) ! sys.env
.get("DOCKER_BUILDKIT")
.map(_ => publishLocalBuildkitLogger(log))
.getOrElse(logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe put the decision logic here. I always find the ! syntax of the sys.process.Process already hard to read and with additional logic behind, it gets even harder.

Suggested change
val logger = publishLocalLogger(log)
val ret = sys.process.Process(buildCommand, context) ! sys.env
.get("DOCKER_BUILDKIT")
.map(_ => publishLocalBuildkitLogger(log))
.getOrElse(logger)
val logger = sys.env.get("DOCKER_BUILDKIT")
.exists(_ == "1")
.map(_ => publishLocalBuildkitLogger(log))
.getOrElse(publishLocalLogger(log))
val ret = sys.process.Process(buildCommand, context) ! logger

Copy link
Author

Choose a reason for hiding this comment

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

That would change the logger for the other cmd running docker images prune.... Not sure this is a good idea ? Or shall I create another variable ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep it consistent. One build kit logger and one if no buildkit is being in use.

So yeah. I'm fine with changing the logger for the prune command if buildkit is enabled

Copy link
Author

Choose a reason for hiding this comment

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

It will potentially output in INFO actual errors from the pruning though. But if you're fine with it, I made the change.

@muuki88
Copy link
Contributor

muuki88 commented Oct 14, 2020

Can you try running scalafmtFormatAll? It checked out your branch and it formatted the DockerPlugin.scala.

Or maybe you have a global scalafmt plugin installed?

@lightbend-cla-validator

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

1 similar comment
@lightbend-cla-validator

At least one pull request committer is not linked to a user. See https://help.github.com/en/articles/why-are-my-commits-linked-to-the-wrong-user#commits-are-not-linked-to-any-user

@muuki88 muuki88 merged commit 8b40a93 into sbt:master Oct 15, 2020
@muuki88
Copy link
Contributor

muuki88 commented Oct 15, 2020

Thanks for fixing this 🤗 I'll release as soon as I got the release process up and running again 😞

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.

BUILDKIT output is logged as error
3 participants