-
Notifications
You must be signed in to change notification settings - Fork 47
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
Environments #185
Environments #185
Conversation
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 haven't done a code review yet, this is just a first pass at the initial concept
@@ -7,13 +7,24 @@ def region | |||
@region ||= ENV['AWS_REGION'] || Aws.config[:region] || Aws.shared_config.region | |||
end | |||
|
|||
def profile_name | |||
@profile_name ||= ENV['AWS_PROFILE'] || Aws.config[:profile_name] | |||
end |
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'd like to see this in a seperate PR to have a seperate conversation there. I'm not convinced this is the right path for StackMaster.
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 could split this out. Do you not think the whole PR is not the right direction or just this change?
Specifying the profile is completely optional. If you leave it out it will just default to the env variables.
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 understand but I believe a Pull request should have one concern and one concern only.
Discussions of this change are likely to confuse the other discussion that needs to happen.
stack_master delete [environment] [stack-name] # Delete a stack | ||
stack_master events [environment] [stack-name] # Display events for a stack | ||
stack_master outputs [environment] [stack-name] # Display outputs for a stack | ||
stack_master resources [environment] [stack-name] # Display outputs for a stack |
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.
Are we changing the core API here? Can you continue to use StackMaster in the traditional way, i.e. by just specifying a region.
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.
there would be nothing stopping you giving environments the same names as the region they are in so yes.
However, yes this whole PR is a major breaking change. The yaml format is similar but different.
Mainly turns things around from stackmaster having regions as it's key data model entry point to environments.
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 feel we are making a change that breaks existing functionality for the benefit of some but not all users. This doesn't feel like the right trade off to me.
I like the idea of environments, I think moving away from region aliases is a good move but this feels like the wrong approach. What about a non breaking change like:
platforms:
myapp:
myapp-vpc:
template: vpc.rb
myapp-elb:
template: elb.rb
myapp-web:
template: web.rb
environments:
production:
us-east-1:
- myapp
eu-central-1:
- myapp
staging:
us-west-2:
- myapp
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.
This is better than what we have, that's for sure. Right now I have no way to deploy the same stack for different environments to the same region.
Closing this for now, we will swing back and have a crack at implementing this when we're ready for a 2.0 release. |
A proof of concept for an implementation based on environments rather than regions.
See #180