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.

Sign up for GitHub

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

Jump to bottom

Fix to return data when using lambda_info module #64548

Merged
merged 8 commits into from Dec 22, 2019
Merged

Fix to return data when using lambda_info module #64548

merged 8 commits into from Dec 22, 2019

Conversation

stefanhorning
Copy link
Contributor

SUMMARY

Fix issue #64479 with lambda_info module.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • lambda_info
  • lambda_facts
ADDITIONAL INFORMATION

Jus opened this PR with the fix suggested by @s-hertel in the ticket #64479 .

@stefanhorning stefanhorning mentioned this pull request Nov 7, 2019
Closed
@ansibot
Copy link
Contributor

ansibot commented Nov 7, 2019

cc @pjodouin
click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Nov 7, 2019

@stefanhorning, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot ansibot added affects_2.10 This issue/PR affects Ansible v2.10 aws bug This issue/PR relates to a bug. cloud community_review In order to be merged, this PR must follow the community review workflow. has_issue module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. small_patch support:community This issue/PR relates to code supported by the Ansible community. labels Nov 7, 2019
@stefanhorning
Copy link
Contributor Author

I will also add some integration tests in a few minutes to catch regressions in the future.

@ansibot
Copy link
Contributor

ansibot commented Nov 7, 2019

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

test/integration/targets/lambda_info/files/mini_lambda.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

test/integration/targets/lambda_info/files/mini_lambda.py:0:0: missing: __metaclass__ = type

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 7, 2019
@ansibot
Copy link
Contributor

ansibot commented Nov 7, 2019

cc @mattclay
click here for bot help

@ansibot ansibot added support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. labels Nov 7, 2019
@stefanhorning
Copy link
Contributor Author

Looks like the integration test still misses AWS permission lambda:GetFunctionConfiguration. Don't know how to fix that, as ./hacking/aws_config/testing_policies/compute-policy.json already contains this permission for Lambda policies, so I am not sure what's still missing to make this work.

@Akasurde
Copy link
Member

Akasurde commented Nov 7, 2019

cc @jillr @s-hertel

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Nov 7, 2019
@s-hertel
Copy link
Contributor

s-hertel commented Nov 7, 2019

@stefanhorning Thanks for adding tests! Here are the policies we actually deploy for CI https://github.com/mattclay/aws-terminator/tree/master/aws/policy. Those were made open to the public after the hacking/ policies were already used by contributors for their own development, so they diverged (and the permissions we use in CI also need to be more strict). You can open a PR to that repo with the permissions that are missing. @jillr Has been reviewing/deploying those on Thursdays.

@mattclay mattclay added the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label Nov 7, 2019
@Akasurde Akasurde changed the title Fixed issue 64479 with lambda_info module Fix to return data when using lambda_info module Nov 8, 2019
@stefanhorning
Copy link
Contributor Author

Ok will look into it, once back from vacation.

@stefanhorning stefanhorning mentioned this pull request Nov 17, 2019
Merged
@stefanhorning
Copy link
Contributor Author

Added missing CI AWS permissions here mattclay/aws-terminator#78

Once deployed the tests should probably run through.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 17, 2019
jillr
jillr reviewed Nov 18, 2019
View reviewed changes
Copy link
Contributor

@jillr jillr 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 this patch and the test cases @stefanhorning. We'll need some additional permissions to run these tests in the CI account though. Would you mind opening a PR in https://github.com/mattclay/aws-terminator? I think lambda:GetFunctionConfiguration, lambda:ListVersionsByFunction, lambda:ListAliases, lambda:ListEventSourceMappings should cover it.

test/integration/targets/lambda_info/files/mini_lambda.py Outdated Show resolved Hide resolved
@stefanhorning
Copy link
Contributor Author

stefanhorning commented Nov 19, 2019 •
edited

@jillr I had already a PR open in aws-terminator mattclay/aws-terminator#78 which I used now to add the additional List permissions you suggested.

Also made the sanity-test related changes here as requested.

@ansibot ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Nov 19, 2019
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Nov 28, 2019
jillr
jillr reviewed Dec 5, 2019
View reviewed changes
Copy link
Contributor

@jillr jillr left a comment

Choose a reason for hiding this comment

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

Tests look good, policy changes have been deployed. One request inline though.

test/integration/targets/lambda_info/aliases Outdated Show resolved Hide resolved
@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Dec 20, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 20, 2019
@stefanhorning
Copy link
Contributor Author

@jillr PR is ready to be merged now.

@s-hertel s-hertel removed the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label Dec 22, 2019
@s-hertel s-hertel merged commit 6fa070e into ansible:devel Dec 22, 2019
@s-hertel
Copy link
Contributor

Thanks @stefanhorning and @jillr!

Akasurde added a commit to Akasurde/ansible that referenced this pull request Dec 30, 2019
@Akasurde
lambda_info: Remove unused variable and import
9226526 
* Clean up residual code from ansible#64548

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@Akasurde Akasurde mentioned this pull request Dec 30, 2019
Merged
s-hertel pushed a commit that referenced this pull request Jan 3, 2020
@Akasurde @s-hertel
lambda_info: Remove unused variable and import (#66124)
5e38d5a 
* Clean up residual code from #64548

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
countless-integers pushed a commit to countless-integers/ansible that referenced this pull request Jan 6, 2020
@stefanhorning @jillr @countless-integers
Fix to return data when using lambda_info module (ansible#64548)
559b524 
* Fixed issue 64479 with lambda_info module

* Added integration tests for lambda_info module

* Moved lambda_info tests into already existing aws_lamda testsuite for easier test setup.

Co-authored-by: Jill R <4121322+jillr@users.noreply.github.com>
countless-integers pushed a commit to countless-integers/ansible that referenced this pull request Jan 6, 2020
@Akasurde @countless-integers
lambda_info: Remove unused variable and import (ansible#66124)
d584181 
* Clean up residual code from ansible#64548

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
@ansible ansible locked and limited conversation to collaborators Jan 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jillr jillr jillr left review comments

Assignees
No one assigned
Labels
affects_2.10 This issue/PR affects Ansible v2.10 aws bug This issue/PR relates to a bug. cloud community_review In order to be merged, this PR must follow the community review workflow. has_issue module This issue/PR relates to a module. small_patch support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@stefanhorning @ansibot @Akasurde @s-hertel @jillr @mattclay