Skip to content

Conversation

wchengru
Copy link
Contributor

@wchengru wchengru commented Jul 5, 2021

Issue #, if available:
#1911
Description of changes:

Description of how you validated changes:

Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation
  • Verify transformed template deploys and application functions as expected

Examples?

Please reach out in the comments, if you want to add an example. Examples will be
added to sam init through https://github.com/awslabs/aws-sam-cli-app-templates/

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

@wchengru wchengru marked this pull request as draft July 5, 2021 00:37
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2021

Codecov Report

Merging #2076 (55d397a) into develop (e7a1496) will increase coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2076      +/-   ##
===========================================
+ Coverage    93.58%   93.92%   +0.34%     
===========================================
  Files           90       92       +2     
  Lines         6124     6273     +149     
  Branches      1260     1278      +18     
===========================================
+ Hits          5731     5892     +161     
+ Misses         183      175       -8     
+ Partials       210      206       -4     
Impacted Files Coverage Δ
samtranslator/model/apigateway.py 97.15% <ø> (ø)
samtranslator/model/sam_resources.py 91.26% <ø> (+0.03%) ⬆️
samtranslator/__init__.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/dialup.py 100.00% <100.00%> (ø)
samtranslator/feature_toggle/feature_toggle.py 100.00% <100.00%> (+12.16%) ⬆️
samtranslator/metrics/metrics.py 100.00% <100.00%> (ø)
samtranslator/model/api/api_generator.py 94.39% <100.00%> (+0.03%) ⬆️
samtranslator/translator/translator.py 97.22% <100.00%> (+0.03%) ⬆️
... and 5 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 09677fc...55d397a. Read the comment docs.

SimpleAccountPercentileDialup,
)

my_path = os.path.dirname(os.path.abspath(__file__))
Copy link
Contributor

Choose a reason for hiding this comment

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

@c2tarun I believe you where the original author of Feature Toggle. Why was this developed in this way? If we remove this, are there any downstream side effects? I assume this was done for a reason, just not sure what it was.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, this is so that we can reference this config file from sam-translate script for testing.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is just for the script, I would rather break that than do patching like this in the lib. We shouldn't need FeatureToggle from that script anyways. Maybe I am missing something but I would error on not overriding sys.path as it's super dangerous.

@moelasmar moelasmar requested a review from c2tarun July 15, 2021 21:43
@jfuss jfuss self-assigned this Mar 28, 2022
@jfuss
Copy link
Contributor

jfuss commented Apr 20, 2022

Closing in favor of #2380

@jfuss jfuss closed this Apr 20, 2022
@jfuss jfuss mentioned this pull request Apr 22, 2022
8 tasks
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.

5 participants