-
-
Notifications
You must be signed in to change notification settings - Fork 607
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
Lint: Fix [301] Commands should not change things if nothing needs doing
#1139
Conversation
roles/common/tasks/reload_nginx.yml
Outdated
@@ -2,6 +2,7 @@ | |||
- name: reload nginx | |||
command: nginx -t | |||
notify: "{{ (role_path | basename == 'common') | ternary('perform nginx reload', omit) }}" | |||
changed_when: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I think this is breaking things. Both in production and with Vagrant Nginx isn't reloading and you just get the default Nginx page. I'll test without it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep this prevents nginx from actually reloading
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we need changed_when: true
to trigger notify
. See: https://serverfault.com/a/799282
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need this as changed_when: true
true? Which I'm guessing fails the lint... any idea what the solution is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed_when: true
passes ansible-lint
and triggers nginx reload. Tested use cases:
vargant up
the first time- change site domains and then
vagrant provision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It leads to the question that should we changed_when: true
on other tasks:
Get list position of current symlinked release
: No because it doesn't change anythingCollect list of releases
: No because it doesn't change anythingGet name of current symlinked release
: No because it doesn't change anythingCreate or close Xdebug SSH tunnel
: Yes because it changes the tunnel. See: 78ebc81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with those 👍
3add785
to
fec1a48
Compare
[301] Commands should not change things if nothing needs doing
[301] Commands should not change things if nothing needs doing
[301] Commands should not change things if nothing needs doing
[301] Commands should not change things if nothing needs doing
No description provided.