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: fix bug in custom DeploymentPreference feature #966

Merged
merged 3 commits into from
Jun 11, 2019

Conversation

keetonian
Copy link
Contributor

@keetonian keetonian commented Jun 11, 2019

Issue #, if available:
N/A
Description of changes:
Supports Fn::FindInMap and other intrinsics for the new custom CodeDeploy configurations feature
Description of how you validated changes:
Tests updated and pass
Checklist:

  • Write/update tests
  • make pr passes
  • Update documentation

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

@keetonian
Copy link
Contributor Author

The work required was actually smaller for this bug than I thought. This should fix this feature and allow the release to go out.

},
"DeploymentConfigName": {
"Fn::Sub": [
"CodeDeployDefault.Lambda${ConfigName}",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an example of the "Sub nested in Sub" case that the code explicitly fixes.

@codecov-io
Copy link

Codecov Report

Merging #966 into release/v1.12.0 will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           release/v1.12.0     #966      +/-   ##
===================================================
+ Coverage            94.66%   94.68%   +0.02%     
===================================================
  Files                   69       69              
  Lines                 3018     3030      +12     
  Branches               562      564       +2     
===================================================
+ Hits                  2857     2869      +12     
  Misses                  85       85              
  Partials                76       76
Impacted Files Coverage Δ
samtranslator/model/sam_resources.py 95.59% <100%> (+0.09%) ⬆️
...el/preferences/deployment_preference_collection.py 91.25% <100%> (+0.34%) ⬆️
samtranslator/translator/translator.py 99% <100%> (+0.03%) ⬆️

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 6615433...35b7357. Read the comment docs.

return value
else:
if value in CODEDEPLOY_PREDEFINED_CONFIGURATIONS_LIST:
print(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

return value
else:
if value in CODEDEPLOY_PREDEFINED_CONFIGURATIONS_LIST:
print(key)
if key == "Fn::Sub": # Don't nest a "Sub" in a "Sub"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this means. Could you add a clearer comment? Also, what about !Sub? Or has it been transformed into Fn::Sub at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been transformed to Fn::Sub at this point.

CfnLint error:
[E1019: Sub validation of parameters] (Sub should be a string or array of 2 items for Resources/CustomWithCondition2DeploymentGroup/Properties/DeploymentConfigName/Fn::If/1/Fn::Sub)

It was doing the following:

            "Fn::Sub": {
              "Fn::Sub": [
                "CodeDeployDefault.Lambda${ConfigName}", 
                {
                  "ConfigName": "AllAtOnce"
                }
              ]
            }

@keetonian keetonian merged commit e76b939 into aws:release/v1.12.0 Jun 11, 2019
@keetonian keetonian deleted the release/v1.12.0 branch August 8, 2019 18:21
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.

None yet

4 participants