Skip to content

fix: Add all files under samcli module#3177

Merged
wchengru merged 6 commits intoaws:developfrom
jfuss:fix/include-all-whl
Aug 20, 2021
Merged

fix: Add all files under samcli module#3177
wchengru merged 6 commits intoaws:developfrom
jfuss:fix/include-all-whl

Conversation

@jfuss
Copy link
Contributor

@jfuss jfuss commented Aug 17, 2021

When we released the sam pipeline set of commands, we didn't update
the MANIFEST.in file. This file controls what files get added or ignored
when the .whl file is created. When we add non python files, we need to
remember to update the MaNIFEST.in file. This commit will auto include
any files within the samcli module but exlcude .pyc files. This might
create a bigger .whl but will make sure the cli works going forward.

I did some quick tests and compared 1.28 .whl to one I produced with this
commit. I did not see any differences in the diff.

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Checklist

  • Add input/output type hints to new functions/methods
  • Write design document (Do I need to write a design document?)
  • Write unit tests
  • Write/update functional tests
  • Write/update integration tests
  • 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.

When we released the `sam pipeline` set of commands, we didn't update
the MANIFEST.in file. This file controls what files get added or ignored
when the .whl file is created. When we add non python files, we need to
remember to update the MaNIFEST.in file. This commit will auto include
any files within the samcli module but exlcude .pyc files. This might
create a bigger .whl but will make sure the cli works going forward.

I did some quick tests and compared 1.28 .whl to one I produced with this
commit. I did not see any differences in the diff.
@jfuss
Copy link
Contributor Author

jfuss commented Aug 17, 2021

I haven't been able to replicate the failing unit test locally. Something is non-deterministic about tests/unit/lib/telemetry/test_metric.py::TestTrackCommand::test_must_record_function_duration

metric.get_data()["duration"],
sleep_duration,
"Measured duration must be in milliseconds and " "greater than equal to the sleep duration",
"Measured duration must be in milliseconds and greater than equal to the sleep duration",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Measured duration must be in milliseconds and greater than equal to the sleep duration",
"Measured duration must be in milliseconds and greater than equal to the sleep duration",

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the duration change break what this test is intended to check? The error message suggests this test is asserting that functions run less than 1 second should be recorded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. My reasoning is that the test name is test_must_record_function_duration, which suggests it's scope it recording the duration. The comment is giving the explanation of why is failed, I don't think the duration actually matters for this test (other than asserting our duration is greater than the sleep_duration).

jfuss and others added 2 commits August 18, 2021 11:49
Update comment from ms to s

Co-authored-by: Cosh_ <CoshUS@users.noreply.github.com>
Co-authored-by: Cosh_ <CoshUS@users.noreply.github.com>
@wchengru wchengru merged commit e35bfe1 into aws:develop Aug 20, 2021
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.

3 participants