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

Improve change tracking #1

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

matt17r
Copy link

@matt17r matt17r commented Dec 12, 2020

While adding Let's Encypt into the Playbook (PR to follow) I decided to see if I could "fix" some of the tasks that were saying things were changing even when they weren't.

This PR includes several different commits that do that. Looking at individual commits will (hopefully) give you context for why I'm suggesting each change.

I kept using the word "idempotent" in commit messages... I think technically most (but not all) of these things were already idempotent, it was just hard to tell because they kept saying they'd made changes even if they hadn't. I'm also not sure I fully understand the meaning of the word so I should probably stop using it 😝

I've tried to test this thoroughly but it takes such a long time to run through the full playbook that I would sometimes start making the next change before I'd finished fully testing the previous one. FWIW I've been testing with Ubuntu LTS 20.04 on Linode (Nanode size) and with Ruby 2.7.2

Use the same salt each time to hash the password.
Probably not a security risk considering the password is in the same 
file...
...but let people choose their own salt in vars.yml anyway
Doesn't get flagged as changed when the only change is a cache update
Sourcing .bashrc is only necessary if you're reusing the current shell?
And Ansible reconnects for every task?
And if both these are true that means this task is unnecessary?
Just to remove a couple of erroneous "changed" notifications
@@ -115,6 +110,9 @@
- redis-tools
- nodejs
- yarn
update_cache: yes
register: apt_output
changed_when: "not '0 upgraded, 0 newly installed, 0 to remove' in apt_output.stdout"
Copy link
Owner

@cupnoodle cupnoodle Dec 12, 2020

Choose a reason for hiding this comment

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

I tried running this script second time on a provisioned server, this part fails as the apt_output dictionary doesn't contain this message.

Screenshot 2020-12-12 at 5 30 15 PM

This is how the apt_output variable looks like when I use debug module to show its output

Screenshot 2020-12-12 at 5 39 54 PM

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm... I thought I had that bit working reliably on both first install and subsequent runs but I thought I got rid of the text search of stdout.

I did keep getting myself mixed up in git trying to keep these changes separate from my own changes separate from let's encrypt changes. Based on your result perhaps the changed_when: isn't necessary?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah I see, I tried running the apt part on a provisioned server, Ansible seems smart enough to not run the apt installation step again, and got a "OK" instead of "changed". I think it should be safe to remove the changed_when here

Copy link
Author

Choose a reason for hiding this comment

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

Cool... do you want me to make those changes?

Also, a referral code for Digital Ocean would be good! I think I've had an account there in the past but it was ages ago and I never used it so no harm trying again.

@cupnoodle
Copy link
Owner

Thanks a lot of the pull request Matthew! I am reviewing it now

`state: reloaded` starts or reloads Nginx
@cupnoodle
Copy link
Owner

cupnoodle commented Dec 12, 2020

The nginx restart handler seems failing on first run, I tried running this on fresh Ubuntu instance on DigitalOcean, it seems to timeout weirdly, I am not sure if this is Ansible or DigitalOcean bug, I think I will revert to manual restart at the end of the step for now

Screenshot 2020-12-12 at 7 38 13 PM

@matt17r
Copy link
Author

matt17r commented Dec 12, 2020

Sorry about the poor testing. To shorten the feedback loop I had a separate playbook that I would use to test one or two tasks against a server that had already had the whole thing run against it from scratch... Once all those individual things were done I'd run "from scratch" again (and wait forever) only to discover there were things that needed to be fixed in that mode of operation. And back and forth it went...

I should have spun up more servers to parallelise the testing a bit but I didn't want to waste my Linode credits 🙂

@cupnoodle
Copy link
Owner

Sorry about the poor testing. To shorten the feedback loop I had a separate playbook that I would use to test one or two tasks against a server that had already had the whole thing run against it from scratch... Once all those individual things were done I'd run "from scratch" again (and wait forever) only to discover there were things that needed to be fixed in that mode of operation. And back and forth it went...

I should have spun up more servers to parallelise the testing a bit but I didn't want to waste my Linode credits 🙂

No worries Matthew! Thanks a lot of the PR, I really appreciate your effort! If you haven't had a DigitalOcean account, I can give you a referral code which you can get $100 credit upon sign up which you can use it for testing.

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.

2 participants