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

Add 'commit messages' for things like emp run, emp scale, emp set #767

Closed
phobologic opened this issue Mar 17, 2016 · 11 comments
Closed

Add 'commit messages' for things like emp run, emp scale, emp set #767

phobologic opened this issue Mar 17, 2016 · 11 comments

Comments

@phobologic
Copy link
Contributor

It'd be good to have some context when these things happen.

Maybe a flag to make it required or not, and a -m option to add it automatically from the command line, or else you get prompted?

I think this is a good set of commands to start with:

  • emp run
  • emp scale
  • emp set

@sanjayprabhu

@mwildehahn
Copy link
Contributor

Discussed a little bit with @phobologic

Rough decisions for v1:

  • empire already supports events, the things we want to track commit messages for are a subset of those events.
  • instead of worrying about storage for v1, add support for adding a commit message to those events

For v2, we can add an alternative event backend that also publishes events to some persistent storage (S3 or RDS etc)

@mwildehahn
Copy link
Contributor

mwildehahn commented Apr 27, 2016

v1:

  • emp cli should support -m for a subset of commands (similar to -a )
  • emp api needs to be able to handle the commit message for those events

v1.1:

  • should have some flag that allows you to require commit messages for a set of actions for the empire server
  • if the server requires a commit message, it should return an error saying commit message is required

v1.2:

  • the client should handle the error from the server and have some UI for seeing if the user wants to enter the required commit message

@ejholmes
Copy link
Contributor

We could store the commit message on the description column on releases, but maybe it'd be better to store it in a separate column.

@phobologic
Copy link
Contributor Author

Yeah, the weird part with storing this is the fact that we are looking for commits in other events that don't create releases, like scaling events.

@ejholmes
Copy link
Contributor

Ah yeah, good call. Although honestly, maybe those should create new releases? The only reason we didn't was to have the same behavior as Heroku.

@sanjayprabhu
Copy link

+1 to scaling events creating releases. Would love to be able to see them
in emp releases.

On Wed, Apr 27, 2016, 15:46 Eric Holmes notifications@github.com wrote:

Ah yeah, good call. Although honestly, maybe those should create new
releases? The only reason we didn't was to have the same behavior as Heroku.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#767 (comment)

Sanjay

@phobologic
Copy link
Contributor Author

I'm waffling on the idea. It feels weird in the 12factor world. I'd almost be more prone to support an 'Events' table and an emp events command that let you look at all the old events, whether for a single app or all of an environment.

@sanjayprabhu
Copy link

Why is it weird? I think it's nice to have the entire application described
by a release. As a contrived example, if you shrink CPU and memory
constraints after a release and then rollback the application won't
actually work as well as before.

On Wed, Apr 27, 2016, 16:50 Michael Barrett notifications@github.com
wrote:

I'm waffling on the idea. It feels weird in the 12factor world. I'd almost
be more prone to support an 'Events' table and an emp events command that
let you look at all the old events, whether for a single app or all of an
environment.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#767 (comment)

Sanjay

@ejholmes
Copy link
Contributor

Yeah, the more I think about this, the more I think scaling should be a new release. Releases should be immutable, but scaling is the only operation that modifies a release.

But, to play devils advocate, there is a performance disadvantage, because creating a new release requires that we submit it to the scheduler, which means all new task definitions across all processes. It may also get weird if ECS ever adds autoscaling, since that wouldn't be reflected in the release.

@phobologic
Copy link
Contributor Author

Just the way we've defined a release in the past - a release is the combination of code & config. Scaling didn't change either of those - you could deploy a release across any sort of scale. They feel like very different things - and I could see us wanting to limit what someone can do in the future. IE: let people scale, but no change config or code.

@mwildehahn
Copy link
Contributor

emp will now support -m for any action that publishes an event.

additionally, you can either provide EMPIRE_MESSAGES_REQUIRED or run the server with --messages.required to require messages for all of these events.

emp will support asking for a message if one is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants