aws-robotics / utils-common

ROS packages for facilitating the use of AWS cloud services.
Apache License 2.0
8 stars 20 forks source link

Remove manual install of importlib #62

Closed dabonnie closed 4 years ago

dabonnie commented 4 years ago

Removed unnecessary manual addition of importlib dependencies, which are not needed for dashing.

Signed-off-by: Devin Bonnie dbbonnie@amazon.com

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

codecov[bot] commented 4 years ago

Codecov Report

Merging #62 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #62   +/-   ##
=======================================
  Coverage   89.60%   89.60%           
=======================================
  Files          18       18           
  Lines         885      885           
=======================================
  Hits          793      793           
  Misses         92       92           
Flag Coverage Δ
#ROS_1 89.60% <ø> (ø)
#ROS_2 91.76% <ø> (ø)
#dashing 91.76% <ø> (ø)
#kinetic 89.69% <ø> (ø)
#melodic 91.76% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9ebc2cb...a0399b4. Read the comment docs.

thomas-moulard commented 4 years ago

LGTM - nit: be careful when renaming workflows. The list of workflows actually blocking the PRs is stored as a string, so if you rename a "required" workflow, it won't be required anymore until someone fixes the repo settings.

dabonnie commented 4 years ago

LGTM - nit: be careful when renaming workflows. The list of workflows actually blocking the PRs is stored as a string, so if you rename a "required" workflow, it won't be required anymore until someone fixes the repo settings.

Good to know, thanks. That's not configured yet, but will configure after merging.

Can you add to the PR description why we're removing this?

Will do.