brew: improve formula to remove deprecated syntax

[Volatus]

Updates the formula to no longer rely on deprecated plist functionality. Fixes the issue with the service failing due to hardcoded PATH variable for the service. Generates the service manifest dynamically instead of embedding it into the formula.

closes #74

Signed-off-by: Ismayil Mirzali ismayilmirzeli@gmail.com

[Volatus]

@rhyeal @mmrwoods

[Volatus]

Thanks @Volatus, this certainly looks an awful lot neater than the old embedded plist.

At a glance, I have a couple of queries though...

• Does this remove the ability to brew install --HEAD, looks like it might, but I am not up to date with homebrew, so maybe there is some default magic nowadays that removes the need to explicitly define where to fetch the head version.
• Does the new service replacement for the old embedded plist work as described on the README, i.e. run the aws-rotate-iam-keys command once for each line defined in ~/.aws-rotate-iam-keys? Again, doesn't look like it does, but I may be missing something obvious.

The plist solution was always a hack to make it easy to define multiple scheduled invocations of the aws-rotate-iam-keys on a mac using launchd rather than cron, it's effectively an embedded script that invokes aws-rotate-iam-keys as many times as defined in the ~/.aws-rotate-iam-keys config file. To be fair to the hack, it works pretty well!

Don't think it'd be too difficult to add back in the logic to support brew install --HEAD.

I'll see what I can do about running it once per line in the file..

[mmrwoods]

Thanks @Volatus, appreciate the contribution:-)

I've also remembered the importance of RunAtLoad in the old plist, which effectively tells the service to run when the user logs in (it is a user launch agent), which is important to ensure keys are rotated even when the computer is asleep at the scheduled run time. I don't know how this is supported in the new homebrew syntax/dsl, but I guess it must be.

And, the old plist also writes to a custom log file in /tmp, because macos unified logging is, to put it politely, a pain in the ass, reading from a simple text file to see error logs is just so much simpler.

There should be useful info in the commit history explaining any other oddites in the plist that might need to be copied across to a refactored solution using the nice new syntax from homebrew. Come to think of it, I remember another one... the check for an internet connection before trying to rotate keys, this was based on experience, frequently aws-rotate-iam-keys would run before the wifi connection was enabled after logging in to my mac in the morning, resulting in failures to rotate keys.

[Volatus]

cc @mmrwoods

I believe just about everything has been addressed. I had to get rid of the pipes to /dev/null due to brew parsing > symbol improperly and I was not able to find a fix for this. However, it shouldn't be a big deal to log those lines. Everything else should be in order.

Also, https://github.com/rhyeal/aws-rotate-iam-keys/pull/71 will not be needed once this PR is merged.

[mmrwoods]

Hi @Volatus, sorry about the delayed response, but thanks for working on this.

I ran into some minor issues testing this which would be nice to address before a merge.

1. run_at_load is new to the Homebrew DSL, so this required a brew update, probably warrants a readme note
2. run_at_load is set to false in the new service definition, but don't we want this to be true? (I assume that otherwise it won't run if the computer is not running at the cron scheduled time)
3. The post install message from Homebrew shows "Or, if you don't want/need a background service you can just run: bash -c if ! curl -s www.google.com; then sleep 60; fi; cp /dev/null /usr/local/var/log/homebrew.mxcl.aws-rotate-iam-keys.log ; ( grep -E ^[[:space:]]*- ~/.aws-rotate-iam-keys || cat /usr/local/etc/aws-rotate-iam-keys ) | while read line; do /usr/local/opt/aws-rotate-iam-keys/bin/aws-rotate-iam-keys $line; done" (maybe it always did, I can't remember, but it strikes me that this should be moved to a script maybe called aws-rotate-iam-keys-daily or something similar?)

[mmrwoods]

Hi @Volatus, a couple of other things I forgot to mention...

I'm unsure of the new log file location, the previous solution intentionally overwrote a file in /tmp daily, primarily to avoid having to even think about log rotation, but this has also sidesteps any potential permissions issues, and ensures the log file is in a consistent location for all installations.

I did notice that the head url has now changed to be fixed to the rhyeal git repo. This is probably a good thing as it's far less brittle, but I think it also makes it impossible to brew install --HEAD from a fork (which is why the previous convoluted, and brittle, code existed).

I'm not particularly wedded to the existing behaviour for either the log file or the --HEAD installs, but these are notable changes all the same.

[Volatus]

I've also remembered the importance of RunAtLoad in the old plist, which effectively tells the service to run when the user logs in (it is a user launch agent), which is important to ensure keys are rotated even when the computer is asleep at the scheduled run time. I don't know how this is supported in the new homebrew syntax/dsl, but I guess it must be.

You're right, I just pushed a fix for this.

[Volatus]

I did notice that the head url has now changed to be fixed to the rhyeal git repo. This is probably a good thing as it's far less brittle, but I think it also makes it impossible to brew install --HEAD from a fork (which is why the previous convoluted, and brittle, code existed).

@mmrwoods

Well I think it's fair to expect that someone forking the repository would also go ahead and change the formula to point to their own repo. In fact I'd say it would be weird for me if I found out that the formula doesn't default to the upstream repo but is dynamic.

I'm unsure of the new log file location, the previous solution intentionally overwrote a file in /tmp daily, primarily to avoid having to even think about log rotation, but this has also sidesteps any potential permissions issues, and ensures the log file is in a consistent location for all installations.

This change I can revert, however it makes sense for something installed with brew to log within the brew log directory for consistency.

[Volatus]

@mmrwoods Any updates on this?

[mmrwoods]

Hi @Volatus, sorry about the radio silence, have been busy with work.

RE the head version being repo aware, I agree this might seem strange, but there was a concrete use case for this... before I had write access to this repo I worked on a fork, which my team as their homebrew tap, and then installed using --HEAD. That allowed us to test out our changes extensively before submitting pull requests back to the upstream repo.

Having said all that, it was very much a hack, so it can be removed. If something like that is needed again, I'll make a better hack :-)

RE the log file location, I can see the appeal of moving it to a log directory under brew --prefix, but there are also problems with this... For a start brew --prefix is not a consistent location, so this makes it slightly trickier to support users of the script, some of whom will have no idea what brew --prefix is, and even getting them to type it into a terminal correctly is going to slow you down (trust me, I've been the person supporting this for a large team of developers and testers). It's also not really intended as a traditional log file, it's a one time debug log, which I added specifically to help me support users of the script, so moving it to a system log directory seems strange; and finally, by just writing to /tmp you simply don't need to worry about permissions or creating missing directories or anything like that, it'll just work, again, if you're supporting this, you want it to just work (I've been there, the less I have to worry about, the better).

[Volatus]

It's also not really intended as a traditional log file, it's a one time debug log, which I added specifically to help me support users of the script, so moving it to a system log directory seems strange; and finally, by just writing to /tmp you simply don't need to worry about permissions or creating missing directories or anything like that, it'll just work, again, if you're supporting this, you want it to just work (I've been there, the less I have to worry about, the better).

Got it @mmrwoods. I'll revert the logging change. And in terms of the testing changes of a fork locally, one can use brew install --debug ./formula-file.rb which is about as simple as using brew install --HEAD tap/formula syntax.

[mmrwoods]

Got it @mmrwoods. I'll revert the logging change. And in terms of the testing changes of a fork locally, one can use brew install --debug ./formula-file.rb which is about as simple as using brew install --HEAD tap/formula syntax.

Thanks for the tip, that's a lot simpler overall!

[Volatus]

@mmrwoods I've reverted the log path to be same as before.

[mmrwoods]

Thanks @Volatus, this looks great, really appreciate the contribution you made here.

I'm happy to merge this, but I don't think it will actually build a new release as there is an issue the circleci config.

@rhyeal Any suggestions? I know like me you are tight for time, in the short term should we just hack this and copy the changes from the template files to the output files?

[Volatus]

@mmrwoods @rhyeal would be nice to get this merged because without the correct PATH changes included in this PR, the script keeps failing for me.

[mmrwoods]

Hi @Volatus, sorry there has been no progress on this. As the build process is currently failing, if you could make the same changes to README.md and Formula/aws-rotate-iam-keys.rb as the corresponding templates I will merge. Thanks

[Volatus]

@mmrwoods I could try fixing the CI instead but it doesn't load the run page for me. (I assume the run being about a year old is a factor)

[mmrwoods]

@Volatus Sorry, I've never tried to run the build process on circleci.