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

Fanout script: Document and make executable #23154

Merged
merged 2 commits into from Jun 19, 2018
Merged

Conversation

islemaster
Copy link
Contributor

While dealing with a low memory page this morning, @sureshc and I spent some time figuring out what the state of our fanout scripts are and realized there's some organizational memory that hasn't been encoded in the scripts themselves. So here are a couple of changes to help with that.

  • Make the scripts executable (644 -> 755)
    This removes the need to make them executable every time you'd like to use them, and also means they'll show up in the shell autocomplete when you're logged in to the server.
  • Document expected usage pattern
    I've removed some very old TODOs and added documentation to each file explaining that we expect these scripts to run on production-daemon and describing a previously-undocumented prerequisite that the appropriate ssh config be copied into place before the script is run. (Apparently we wipe this config on every deploy.)

I think these are useful, safe changes to make on their own. I'm also interested in some discussion of other changes we could make here.

  • @wjordan has a very old pull request that started work on the TODO I'm removing, adding a fanout method to RakeUtils.
  • It seems like fanout-dryrun and fanout-parial ought to be flags on the main fanout script, both because it's DRY, and because right now I don't have confidence that dryrun will actually represent the behavior of fanout without doing a diff of the two scripts.
  • Is running the command "for real" on console actually a good dry-run option?

Copy link
Contributor

@ewjordan ewjordan left a comment

Choose a reason for hiding this comment

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

LGTM - will address your comments in another comment.

@ewjordan
Copy link
Contributor

I agree that dryrun and partial should be flags rather than separate files; I don't remember how it ended up this way, I suspect we intended to clean everything up after HoC but got distracted and just copied the working files into Git as they were so we didn't lose them. I believe the PR you mentioned was a start of that cleanup, but we didn't ever get around to getting the Chef stuff resolved, so put off finalizing it.

Is running the command "for real" on console actually a good dry-run option?
I'm not exactly sure what you mean by this - what would be the alternative?

@islemaster
Copy link
Contributor Author

I guess dry-run is a fairly specialized concept depending on the command that you're talking about, but my instinct when I first saw that name was that it would do nothing, except maybe print the list of servers it intended to run the command on. I suppose in our setup running commands on console is more useful as a dry-run though.

@ewjordan
Copy link
Contributor

Oh - I just realized that I hadn't looked at that file (the dryrun one). That does seem like odd behavior, I don't remember why we did that at all. I'm totally fine with changing or removing it.

@islemaster islemaster merged commit edbaed7 into staging Jun 19, 2018
@islemaster islemaster deleted the fanout-executable branch June 19, 2018 16:29
@wjordan wjordan mentioned this pull request Jan 7, 2020
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.

None yet

2 participants