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

Correct runtime error during cleanup. #429

Merged
merged 4 commits into from
Jul 29, 2024

Conversation

pecastro
Copy link
Contributor

@pecastro pecastro commented May 27, 2024

TASK [ansible-role-nginx-config : Remove NGINX config files] *******************************************************************************************************************************************************
Monday 27 May 2024 11:53:13 +0100 (0:00:00.029) 0:00:14.736 ************
fatal: [localhost]: FAILED! => {"msg": "'dict object' has no attribute 'results'. 'dict object' has no attribute 'results'"}

Proposed changes

Describe the use case and detail of the change. If this PR addresses an issue on GitHub, make sure to include a link to that issue using one of the supported keywords here in this description (not in the title of the PR).

Checklist

Before creating a PR, run through this checklist and mark each as complete:

  • I have read the CONTRIBUTING document.
  • I have added Molecule tests that prove my fix is effective or that my feature works.
  • I have checked that any relevant Molecule tests pass after adding my changes.
  • I have updated any relevant documentation (defaults/main/*.yml, README.md and CHANGELOG.md).

@pecastro pecastro requested a review from alessfg as a code owner May 27, 2024 11:25
Copy link

github-actions bot commented May 27, 2024

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
Posted by the CLA Assistant Lite bot.

@pecastro
Copy link
Contributor Author

recheck

@pecastro
Copy link
Contributor Author

I have hereby read the F5 CLA and agree to its terms

@pecastro
Copy link
Contributor Author

recheck

@alessfg alessfg added the bug Something isn't working label May 27, 2024
@alessfg alessfg added this to the 0.7.2 milestone May 27, 2024
@pecastro
Copy link
Contributor Author

recheck

@pecastro
Copy link
Contributor Author

The error https://github.com/nginxinc/ansible-role-nginx-config/actions/runs/9254016965/job/25467668666?pr=429#step:6:375
seems to be related to the NGINX certificate which is totally unrelated to the fix contained in the this MR.

@alessfg
Copy link
Collaborator

alessfg commented May 28, 2024

You are right - I recently expanded the suite of tests and I forgot to exclude some tests from external PRs. I'll submit a fix asap.

In the meantime I'd appreciate if you could make a couple updates to your PR 😄

tasks/config/cleanup-config.yml Outdated Show resolved Hide resolved
@alessfg
Copy link
Collaborator

alessfg commented May 29, 2024

Tests should be passing!

@pecastro
Copy link
Contributor Author

recheck

Copy link
Collaborator

@alessfg alessfg left a comment

Choose a reason for hiding this comment

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

Can you fix the linter error and remove the double quotes surrounding the conditionals? And can you update the Changelog? Thanks! I forgot to mention that in my previous review 😅

@pecastro pecastro force-pushed the fix-clenaup-file-error branch 2 times, most recently from f45521f to b3e216e Compare May 29, 2024 19:22
TASK [ansible-role-nginx-config : Remove NGINX config files] *******************************************************************************************************************************************************
Monday 27 May 2024  11:53:13 +0100 (0:00:00.029)       0:00:14.736 ************
fatal: [localhost]: FAILED! => {"msg": "'dict object' has no attribute 'results'. 'dict object' has no attribute 'results'"}
@pecastro
Copy link
Contributor Author

recheck

@pecastro
Copy link
Contributor Author

@alessfg What are we missing here? Pipeline doesn't move on ...

@alessfg
Copy link
Collaborator

alessfg commented May 31, 2024

I need to manually run the pipeline for external PRs! The recheck comment only works with the CLA action :)

@alessfg alessfg enabled auto-merge (squash) May 31, 2024 16:56
@alessfg
Copy link
Collaborator

alessfg commented May 31, 2024

FYI, there was an update to the nginx signing keys two days ago and until nginxinc/ansible-role-nginx#719 is merged and this role updated to use the latest commit, the pipeline here will fail.

@pecastro
Copy link
Contributor Author

pecastro commented Jun 1, 2024

nginxinc/ansible-role-nginx#719 has been merged and I don't see anywhere in the code a reference to that repo. 🤷

@alessfg
Copy link
Collaborator

alessfg commented Jun 2, 2024

The role installs the latest release of the NGINX role as part of the CI/CD pipeline (see https://github.com/nginxinc/ansible-role-nginx-config/blob/main/molecule/common/requirements/oss_requirements.yml). A new release needs to be created and this role updated to use that release before tests here will pass.

@pecastro
Copy link
Contributor Author

pecastro commented Jun 3, 2024

I get you now. So this is blocked.

@alessfg alessfg disabled auto-merge July 29, 2024 00:20
@alessfg alessfg merged commit 935472f into nginxinc:main Jul 29, 2024
12 checks passed
@alessfg
Copy link
Collaborator

alessfg commented Jul 29, 2024

Finally got around merging this. Thanks again for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

None yet

2 participants