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

Tail CloudFormation events #146

Merged
merged 9 commits into from
Sep 22, 2017
Merged

Tail CloudFormation events #146

merged 9 commits into from
Sep 22, 2017

Conversation

taraspos
Copy link
Contributor

This implements PR #143.

New command added:
awless tail stack-events

By default shows last N (10 default) events.

Flag --follow allow following events with refresh interval N seconds (10 default).
Local flag --watch-deployment allow following events of current in progress deployment and exits when deployment finishes., if no running deployment exits with an error.


type stackEvents []*cloudformation.StackEvent

func coloredResourceStatus(str string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

A detail but, helpers/boilerplates code should go at the end of the file. We should prefer strong public/business logic at the top of the file.


}

func (e stackEvents) printReverse(w io.Writer) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for this method (and again detail) that can go at the end of file to avoid cognitive effort on non interesting method each time we open this file.

}

func (e stackEvents) printReverse(w io.Writer) error {
tab := tabwriter.NewWriter(w, 35, 8, 0, '\t', 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the space between data when displaying in the console is wide. If done on purpose, fine, otherwise let's minimize it.

@simcap
Copy link
Contributor

simcap commented Sep 22, 2017

Great, thanks! Looks good. We are testing it locally and I am just reviewing some minor details.

ticker := time.NewTicker(t.refreshFrequency)
defer ticker.Stop()
for range ticker.C {
if t.watchDeployment {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not fully seeing the value of the watchDeployment. Couldn't we always have the if clause (i.e. see if the deployment is finished) so that we can always exit and just print to Std.out to let know the user before exiting the CLI. The existing --follow on its own could then express the fact that we are watching the deployment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense.

How do you see follow behave if no deployment running? I would like to still throw an error since I don't see the point of following if nothing happens, this makes sense in terms of usage with the CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Good idea. An explicit friendly error. You can use the exitOn(myerr) method for that.

@simcap
Copy link
Contributor

simcap commented Sep 22, 2017

Our first tailer code was not ideal! So i did some minor corrections in our first tailer in master that you can mimic. See commit 0b03168

(ex: notably not using the term refresh, which should be follow instead since this is the term transferred from the CLI flags)

@fxaguessy
Copy link
Contributor

Also, could you please have a look at https://goo.gl/forms/1lQFPEIxdt37aDn43 (very short form), it's always interesting for us to know the real life usage of awless.
Thanks

@taraspos
Copy link
Contributor Author

@simcap done

@simcap simcap merged commit 3b932cf into wallix:master Sep 22, 2017
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