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.

Sign up for GitHub

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

Jump to bottom

Stack execution progress #81

Merged
merged 1 commit into from
Feb 27, 2018
Merged

Stack execution progress #81

merged 1 commit into from
Feb 27, 2018

Conversation

maxiwoj
Copy link
Contributor

@maxiwoj maxiwoj commented Feb 18, 2018 •
edited by afronski
Loading

This closes #17.

@afronski
Copy link
Contributor

Please resolve conflicts first and attach some screens if possible. :)

afronski
afronski reviewed Feb 18, 2018
View reviewed changes
Copy link
Contributor

@afronski afronski left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍, minor remarks.

main.go Outdated
@@ -77,4 +78,14 @@ func main() {
stack.DestroyStack(&context)
os.Exit(0)
}

if *context.CliArguments.Mode == cliparser.SetupSinkMode {
notificationservice.ConfigureRemoteSink(&context)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rename it to notifications or progress.

notificationservice/notification.go Outdated
TopicArn *string
}

var sinkName = "Perun-sink-"
Copy link
Contributor

Choose a reason for hiding this comment

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

Start with a small letter.

notificationservice/notification.go Outdated
shouldSNSTopicBeRemoved := snsTopicExists && !sqsQueueExists
err = conn.deleteRemainingSinkResources(shouldSNSTopicBeRemoved, false)
if err != nil {
println("error deleting up remote sink", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use logger for errors everywhere in that review.

notificationservice/notification.go Outdated

if err == nil {
context.Logger.Info("Remote sink configuration successful")
context.Logger.Info("Remote Sink configuration may take up to 2 minutes, wait before calling 'create-stack' with flag --progress")
Copy link
Contributor

Choose a reason for hiding this comment

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

Replate Remote Sink configuration with It if it is displayed one after another.
And even better - use warning for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add it then. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe our logger does not have a warning type

notificationservice/notification.go Outdated
}
}

// Remove all AwS Resources created for stack monitoring
Copy link
Contributor

Choose a reason for hiding this comment

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

AwS -> AWS.

notificationservice/notification.go Outdated
conn.context.Logger.Error("error reading json message" + err.Error())
}

//DELETE READ MESSAGE (to prevent reading the same message multiple times)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space after //.

notificationservice/notification.go Outdated
messageMap[messages[0]] = messages[1]
}
// Parse timestamp of message
messageArrivedTime, err := time.Parse("2006-01-02T15:04:05.000Z", v.Timestamp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to name that date and time format with some meaningful name as a constant?

notificationservice/notification.go Outdated
}

func (conn *Connection) createJsonPolicy() (jsonStringPolicy string, err error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Superfluous empty line.

notificationservice/notification.go Outdated
return
}

func (conn *Connection) deleteSnsTopic() (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see deleting a policy here - is it taken care when deleting SQS queue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are parameters of the SQS queue, the policy itself is not created - only set up as an attribute of the SQS queue.

@afronski afronski added this to the Cumulus - 2.0 milestone Feb 18, 2018
@afronski
Copy link
Contributor

One more thing: please create a new branch from current master called, e.g., release_2_0 and target this pull request into that branch, not directly into master. This is because we would like to have this feature in 2.0, not in 1.1.

@Appliscale Appliscale deleted a comment from maxiwoj Feb 27, 2018
@Appliscale Appliscale deleted a comment from maxiwoj Feb 27, 2018
@Appliscale Appliscale deleted a comment from maxiwoj Feb 27, 2018
@Appliscale Appliscale deleted a comment from maxiwoj Feb 27, 2018
@Appliscale Appliscale deleted a comment from maxiwoj Feb 27, 2018
afronski
afronski approved these changes Feb 27, 2018
View reviewed changes
README.md Outdated

To destroy remote sink just type:

``~ $ perun destroy-remote-sink``
Copy link
Contributor

Choose a reason for hiding this comment

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

Use triple backticks and new lines always.

README.md Outdated
##### Remote sink
To setup remote sink type:

```~ $ perun setup-remote-sink```
Copy link
Contributor

Choose a reason for hiding this comment

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

Use newlines inside code blocks.

cliparser/cliparser.go Outdated
@@ -91,7 +91,7 @@ func ParseCliArguments(args []string) (cliArguments CliArguments, err error) {
deleteStack = app.Command(DestroyStackMode, "Deletes a stack on aws")
deleteStackName = deleteStack.Arg("stack", "An AWS stack name.").Required().String()

setupSink = app.Command(SetupSinkMode, "Sets up resources required for progress report on stack events (SNS Topic, SQS Queue)")
setupSink = app.Command(SetupSinkMode, "Sets up resources required for progress report on stack events (SNS Topic, SQS Queue and SQSQueue Policy)")
Copy link
Contributor

Choose a reason for hiding this comment

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

SQSQueue -> SQS Queue.

@Appliscale Appliscale deleted a comment from maxiwoj Feb 27, 2018
@maxiwoj maxiwoj changed the base branch from master to release_2_0 February 27, 2018 10:39
Stack Execution Progress #17
7b5fedc 
Added support for SNS and SQS notifications - remote sink creation and deletion. Untested

Stack Execution Progress #17

Added setup/destroy remote sink for creation of required resources

Stack Execution Progress #17

Added Table with Tablewriter

Stack Execution Progress #17

Added Table Printing

Stack Execution Progress #17

Added verification for current process stack messages and auto close

Stack Execution Progress #17

Fixed Pull request issues

Stack Execution Progress #17

Fixed Pull request issues
@maxiwoj maxiwoj merged commit aecc242 into release_2_0 Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afronski afronski afronski approved these changes

Assignees

@maxiwoj maxiwoj

Labels
None yet
Projects
None yet
Milestone
Cumulus - 2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@maxiwoj @afronski