Skip to content

Conversation

@sriram-mv
Copy link
Contributor

@sriram-mv sriram-mv commented Feb 2, 2023

  • Additive permissions to add execute on directories and read on files.

Which issue(s) does this change fix?

Why is this change necessary?

  • This change makes the permissions additive on files and directories.

How does it address the issue?

  • Only adds permissions and does not take away existing ones.

What side effects does this change have?

  • N/A

Mandatory Checklist

PRs will only be reviewed after checklist is complete

  • Add input/output type hints to new functions/methods
  • Write design document if needed (Do I need to write a design document?)
  • Write/update unit tests
  • Write/update integration tests
  • Write/update functional tests if needed
  • make pr passes
  • make update-reproducible-reqs if dependencies were changed
  • Write documentation

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

@sriram-mv sriram-mv requested a review from a team as a code owner February 2, 2023 06:40
@sriram-mv sriram-mv marked this pull request as draft February 2, 2023 06:40
@sriram-mv sriram-mv marked this pull request as ready for review February 2, 2023 21:35
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

Probably out of scope for this PR but I think it would be a good idea to add integration tests for a compiled and interpreted language that package, deploy, and then invoke the function. I will leave that up to you but at the very least, I think we should add that before this is released.

Copy link
Contributor

@qingchm qingchm left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mndeveci mndeveci left a comment

Choose a reason for hiding this comment

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

Thanks for the change! I've requested some small changes to make it more readable. I also agree for adding an integration test to verify this behavior which would also help us to not break this in the future.

@sriram-mv sriram-mv force-pushed the reintroduce_package_permissions branch 2 times, most recently from 67827a3 to c8e78d5 Compare February 5, 2023 20:38
@sriram-mv sriram-mv force-pushed the reintroduce_package_permissions branch 2 times, most recently from 599f0ab to 2e56ea6 Compare February 5, 2023 21:55
@sriram-mv sriram-mv force-pushed the reintroduce_package_permissions branch from 2e56ea6 to c961fb7 Compare February 5, 2023 21:56
@sriram-mv sriram-mv enabled auto-merge (squash) February 15, 2023 00:22
@sriram-mv sriram-mv merged commit a29a293 into aws:develop Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants