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

fix(core): asset bundling fails for non-existent user #15313

Merged

Conversation

akuntsch
Copy link

@akuntsch akuntsch commented Jun 25, 2021

Currently, when not explicitly passing a user in BundlingOptions, os.userInfo() is called as a default fallback. Even though the user is only used and needed when using docker bundling, it is still called when using local bundling. This can be problematic e.g. when running cdk scripts inside a docker container where the user does not exist inside the container (see #15415).

Fixes #15415.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Jun 25, 2021

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 8, 2021

Please don't forget to describe the bug you are fixing

rix0rrr
rix0rrr previously requested changes Jul 8, 2021
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Build is failing, and needs a description appropriate for the changelog. Why would someone reading the changelog care about this change?

The user is only needed when bundling via Docker so there is no need to always call os.userInfo() as a default fallback even when bundling locally.

Additionally, calling os.userInfo() can easily fail when already running inside a Docker container with non-existent uid and gid (e.g. when using the Jenkins Docker plugin).
@akuntsch akuntsch force-pushed the fix-local-bundling-with-non-existent-user branch from 6f50ee9 to 1c66380 Compare July 30, 2021 09:41
@mergify mergify bot dismissed rix0rrr’s stale review July 30, 2021 09:41

Pull request has been modified.

@akuntsch akuntsch changed the title fix(asset-staging): call os.userInfo() only when not bundling locally fix(asset-staging): local asset bundling fails with non-existent user Jul 30, 2021
@akuntsch
Copy link
Author

@rix0rrr I've updated the PR title and description and the build was successful this time around.

@rix0rrr rix0rrr changed the title fix(asset-staging): local asset bundling fails with non-existent user fix(core): local asset bundling fails with non-existent user Aug 10, 2021
@rix0rrr rix0rrr changed the title fix(core): local asset bundling fails with non-existent user fix(core): asset bundling fails for non-existent user Aug 10, 2021
@rix0rrr rix0rrr added the pr-linter/exempt-test The PR linter will not require test changes label Aug 11, 2021
@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit bf5882f into aws:master Aug 11, 2021
@mergify
Copy link
Contributor

mergify bot commented Aug 11, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 6728027
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@akuntsch akuntsch deleted the fix-local-bundling-with-non-existent-user branch August 12, 2021 07:07
hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
Currently, when not explicitly passing a user in BundlingOptions, `os.userInfo()` is called as a default fallback. Even though the user is only used and needed when using docker bundling, it is still called when using local bundling. This can be problematic e.g. when running cdk scripts inside a docker container where the user does not exist inside the container (see aws#15415).

Fixes aws#15415.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
david-doyle-as24 pushed a commit to david-doyle-as24/aws-cdk that referenced this pull request Sep 7, 2021
Currently, when not explicitly passing a user in BundlingOptions, `os.userInfo()` is called as a default fallback. Even though the user is only used and needed when using docker bundling, it is still called when using local bundling. This can be problematic e.g. when running cdk scripts inside a docker container where the user does not exist inside the container (see aws#15415).

Fixes aws#15415.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(asset-staging): local bundling fails inside a docker container with non-existent user
4 participants