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

Refactor paramater overriding. Set default parameter file #231

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pingram3030
Copy link
Contributor

So, I'm not 100% happy with this because it doesn't feel like the best writing of it, but it's been a while since I had to type out some ruby and some how I managed to fix a rubocop override! :D

We automatically load files from other folders inside of moonshot/, so why not also try and load a parameter file? If there is a better way of doing this I'd be welcoming of feedback, or if someone wants to Nike it that'd be fine also.

@askreet
Copy link
Contributor

askreet commented Aug 15, 2017

I think the implementation looks good and I'm not against adding this feature since using it would be optional but I wonder what use cases you have for this. Prior to the big 2.x refactor we had environment configurations stored locally, and encouraged people to keep them committed to their source repo in cloud_formation/#{environment}.yml.

The goal of the 2.x refactor was to keep AWS as the "source of truth" when performing updates, so we merge the state of AWS into the UpdateStack call with any new changes (passed by -a/-P, etc.) -- why keep a local copy by the name of an environment?

I guess one use case I could see would be having a staging or QE environment that you routinely spin up/down and not having to pass -a moonshot/staging.yml for example.

@pingram3030
Copy link
Contributor Author

It's mainly the last reason. I am working on building out several different environments,
so the churn of updated AMIs, and the need to elevate them for Q[A|E] is rather frequent. I suppose that I could just -P all the things, but having controlled yaml files makes it a touch easier to ensure consistency for other less savvy colleagues. Plus, having the AMI defined in a file makes it easier to C&P between dev and staging configs; there is also the "we have to rebuild the stack now" case where you need to have the params recorded somewhere prior to stack tear down to ensure you recreate it with the same params.

Across environments we change the IP addressing ranges to avoid conflicts between peered stacks and
we also define secrets, hosted zone names, snapshot IDs, etc. (we .gitignore moonshot/params)

It is advantageous for us to control and deploy answer files that contain a lot of the gubbins devs don't need to know about. It means we can just say "If you update AMI or what not, use -P", because we know the defaults in answer_file are sane. By having and controlling the answer file we also make it easy for people to rebuild a stack knowing that when creating a stack that all mandatory params, and some optionals, are sanely set by default.

I now have concerns though over a person having a param file, specifying a new AMI with -P, then just running a regular 'update'. Since the AMI is set in param_file this manual override will be overridden. There is an "are you sure" prompt where all of the changes are detailed, so maybe that's enough, but I now see that this changes some established behaviour.

@jfarrell
Copy link
Contributor

what about creating something like AlwaysUseDefaultSource, but for the answer_file. This way it would not break existing behavior, but for anyone wanting to use a default answer_file based on environment name they could set it in their Moonfile. We currently do something like this using a utility class

@pingram3030
Copy link
Contributor Author

How do you feel about me adding param_dir to Moonfile.rb, updating the method I made to suit and putting something in to docs/user-guide/include_in_your_project.md? I feel 'param_dir' is a bit more intuitive and means the user can put their per environment yaml where ever they please.

@askreet
Copy link
Contributor

askreet commented Aug 16, 2017

@pingram3030 I actually prefer the by-convention approach of this patch to asking users to set it all up. Let's merge this patch in as-is, I just want to give it a manual test before I hit the green button.

If you want to add some docs showing how one might use this feature, that'd be great too.

# The order we override parameters in is, in order of least to most important:
# * project_root/moonshot/params/#{environment}.yml
# * file defined by the user with -a/--answer-file
# * individual parameters the user input with -P/--parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

@pingram3030 Actually I was just thinking about this, does this mean that:

# 1. create stack with known param file containing 'FooParameter'
moonshot create -n staging

# 2. update FooParameter, -P takes precedence here
moonshot update -n staging -P FooParameter=newValue

# 3. update stack with new cloud formation template
moonshot update -n staging

I think that #3 would return the value of FooParameter to the value in the answer file the was changed by #2. If so, this breaks the pattern of allowing configuration change to live in AWS. We could change the algorithm to only use the by-name answer file for creation, and only use it for updates when a parameter is missing (such as a newly added parameter in an updated stack revision).

@pingram3030
Copy link
Contributor Author

I'll have it only check for the param file when creating. I'll definitely add to the doc for this, but I need time to do that; I've been hella starved of free time of late. Hopefully I can get back to this soon, it's something I'd like to have :)

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.

3 participants