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

lambda_info - refactor to fix bug when querying all lambdas #1152

Conversation

jatorcasso
Copy link
Contributor

@jatorcasso jatorcasso commented May 19, 2022

Depends-On: ansible/ansible-zuul-jobs#1558

SUMMARY
  • Fix bug that forces query: config when getting info for all lambdas. Refactored to return the expected info
  • Add extra cleanup at end of tests

Fixes #1151

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lambda_info

ADDITIONAL INFORMATION

This module also currently returns a dict of dicts (as opposed to a list of dicts), but I wanted to keep the scope of this PR to fixing the bug.

@ansibullbot
Copy link

@ansibullbot ansibullbot added bug This issue/PR relates to a bug community_review integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels May 19, 2022
@github-actions
Copy link

github-actions bot commented May 19, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

ansible-galaxy-importer FAILURE in 4m 39s (non-voting)
✔️ build-ansible-collection SUCCESS in 4m 47s
✔️ ansible-test-sanity-docker-devel SUCCESS in 11m 33s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 11m 27s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 15m 19s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 11m 51s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 9m 26s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 5m 57s
✔️ ansible-test-splitter SUCCESS in 2m 29s
integration-community.aws-1 FAILURE in 8m 58s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

ansible-galaxy-importer FAILURE in 4m 01s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 27s
✔️ ansible-test-sanity-docker-devel SUCCESS in 11m 07s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 11s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 12m 51s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 10m 57s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 06s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 01s
✔️ ansible-test-splitter SUCCESS in 2m 45s
integration-community.aws-1 FAILURE in 9m 35s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@jatorcasso
Copy link
Contributor Author

async causing the problem:

ASYNC FAILED on testhost: jid=82723908828.1746
fatal: [testhost]: FAILED! => {
    "ansible_job_id": "82723908828.1746",
    "changed": false,
    "finished": 1,
    "invocation": {
        "module_args": {
            "_async_dir": "/root/.ansible_async",
            "jid": "82723908828.1746",
            "mode": "status"
        }
    },
    "msg": "could not find job",
    "results_file": "/root/.ansible_async/82723908828.1746",
    "started": 1,
    "stderr": "",
    "stderr_lines": [],
    "stdout": "",
    "stdout_lines": []
}

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Depending on how many lambdas people have, this is potentially going to massively increase the default runtime when listing all lambdas.

I agree that only supporting the minimal query is definitely wrong (unless we make name and query mutually exclusive), however, based on my experience with other APIs, flipping the switch so that it defaults to making almost an order of magnitude more queries is likely to have a significant negative effect for anyone with a larger setup.

If this is to be backported I'd want to see the current behaviour preserved:

  • default to "query=config" when the function isn't specified
  • default to "query=all" when the function is specified
    BUT, if query is explicitly set (currently 'all' is a default, so be careful), then that should override the default behaviour.

I'm approving for 4.0.0, but want to be clear I do not think the current PR should be backported.

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Switching to a "Request changes" because I don't think the current change fragment is 'right'

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

ansible-galaxy-importer FAILURE in 3m 39s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 21s
✔️ ansible-test-sanity-docker-devel SUCCESS in 12m 28s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 18s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 10m 51s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 9m 33s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 9m 21s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 5m 35s
✔️ ansible-test-splitter SUCCESS in 2m 36s
integration-community.aws-1 FAILURE in 10m 37s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

Copy link
Collaborator

@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.

I think this looks reasonable, with @tremble's comments noted

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

ansible-galaxy-importer FAILURE in 5m 18s (non-voting)
✔️ build-ansible-collection SUCCESS in 7m 31s
ansible-test-sanity-docker-devel RETRY_LIMIT in 6m 05s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 15m 11s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 18m 51s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 13m 08s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 06s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 8m 24s
✔️ ansible-test-splitter SUCCESS in 3m 24s
integration-community.aws-1 FAILURE in 9m 15s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@goneri
Copy link
Member

goneri commented May 25, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

This change depends on a change that failed to merge.

@tremble
Copy link
Contributor

tremble commented May 25, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

ansible-galaxy-importer FAILURE in 4m 31s (non-voting)
✔️ build-ansible-collection SUCCESS in 4m 55s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 12s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 38s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 14m 35s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 11m 21s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 9m 14s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 5m 47s
✔️ ansible-test-splitter SUCCESS in 2m 26s
integration-community.aws-1 FAILURE in 8m 57s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@goneri
Copy link
Member

goneri commented May 26, 2022

recheck

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

ansible-galaxy-importer FAILURE in 4m 18s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 31s
✔️ ansible-test-sanity-docker-devel SUCCESS in 9m 29s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 11m 51s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 12m 45s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 9m 47s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 10m 41s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 7m 44s
✔️ ansible-test-splitter SUCCESS in 2m 38s
✔️ integration-community.aws-1 SUCCESS in 8m 02s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@jatorcasso jatorcasso closed this May 26, 2022
@jatorcasso jatorcasso reopened this May 26, 2022
@jatorcasso
Copy link
Contributor Author

@tremble can we backport this now with the updated changes?

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

ansible-galaxy-importer FAILURE in 5m 02s (non-voting)
✔️ build-ansible-collection SUCCESS in 5m 13s
✔️ ansible-test-sanity-docker-devel SUCCESS in 11m 44s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 30s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 10m 42s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 9m 45s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 35s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 15s
✔️ ansible-test-splitter SUCCESS in 2m 29s
✔️ integration-community.aws-1 SUCCESS in 7m 59s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@tremble tremble added mergeit Merge the PR (SoftwareFactory) backport-3 PR should be backported to the stable-3 branch labels Jun 1, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

ansible-galaxy-importer FAILURE in 4m 03s (non-voting)
✔️ build-ansible-collection SUCCESS in 4m 50s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 42s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 10m 13s
✔️ ansible-test-sanity-docker-stable-2.9 SUCCESS in 14m 54s
✔️ ansible-test-sanity-docker-stable-2.11 SUCCESS in 12m 00s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 11m 23s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 48s
✔️ ansible-test-splitter SUCCESS in 3m 02s
✔️ integration-community.aws-1 SUCCESS in 9m 02s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 0c76aed into ansible-collections:main Jun 1, 2022
@patchback
Copy link

patchback bot commented Jun 1, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/0c76aedd4700a85ad6ee6971ffb787f2a2df1c2b/pr-1152

Backported as #1188

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jun 1, 2022
lambda_info - refactor to fix bug when querying all lambdas

Depends-On: ansible/ansible-zuul-jobs#1558
SUMMARY

Fix bug that forces query: config when getting info for all lambdas. Refactored to return the expected info
Add extra cleanup at end of tests

Fixes #1151
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
lambda_info
ADDITIONAL INFORMATION
This module also currently returns a dict of dicts (as opposed to a list of dicts), but I wanted to keep the scope of this PR to fixing the bug.

Reviewed-by: Mark Chappell <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Jill R <None>
(cherry picked from commit 0c76aed)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Jun 3, 2022
…1188)

[PR #1152/0c76aedd backport][stable-3] lambda_info - refactor to fix bug when querying all lambdas

This is a backport of PR #1152 as merged into main (0c76aed).
Depends-On: ansible/ansible-zuul-jobs#1558
SUMMARY

Fix bug that forces query: config when getting info for all lambdas. Refactored to return the expected info
Add extra cleanup at end of tests

Fixes #1151
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
lambda_info
ADDITIONAL INFORMATION
This module also currently returns a dict of dicts (as opposed to a list of dicts), but I wanted to keep the scope of this PR to fixing the bug.

Reviewed-by: Mark Chappell <None>
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
…collections#1152)

lambda_info - refactor to fix bug when querying all lambdas

Depends-On: ansible/ansible-zuul-jobs#1558
SUMMARY

Fix bug that forces query: config when getting info for all lambdas. Refactored to return the expected info
Add extra cleanup at end of tests

Fixes ansible-collections#1151
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
lambda_info
ADDITIONAL INFORMATION
This module also currently returns a dict of dicts (as opposed to a list of dicts), but I wanted to keep the scope of this PR to fixing the bug.

Reviewed-by: Mark Chappell <None>
Reviewed-by: Joseph Torcasso <None>
Reviewed-by: Jill R <None>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections@0c76aed
abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
Use missing_required_lib more consistently

SUMMARY
The developer docs recomment using missing_required_lib rather than a custom error message:
https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_best_practices.html#importing-and-using-shared-code
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
plugins/inventory/aws_ec2.py
plugins/inventory/aws_rds.py
plugins/lookup/aws_account_attribute.py
plugins/lookup/aws_secret.py
plugins/lookup/aws_ssm.py
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3 PR should be backported to the stable-3 branch bug This issue/PR relates to a bug community_review integration tests/integration mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lambda_info - only queries config info when function_name is omitted
5 participants