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

R4R: runsim pushes logs to S3 bucket #4677

Merged
merged 7 commits into from
Jul 4, 2019
Merged

R4R: runsim pushes logs to S3 bucket #4677

merged 7 commits into from
Jul 4, 2019

Conversation

mircea-c
Copy link

@mircea-c mircea-c commented Jul 3, 2019

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: clog add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Should this live in the devops/tooling repo? Also, where is the documentation on where/how to fetch these logs in S3? Who has access?

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

minor comments 👍

contrib/runsim/main.go Outdated Show resolved Hide resolved
contrib/runsim/main.go Outdated Show resolved Hide resolved
contrib/runsim/main.go Outdated Show resolved Hide resolved
)

func awsErrHandler(err error) {
func awsErrHandler(err error) error {
if awsErr, ok := err.(awserr.Error); ok {
switch awsErr.Code() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this switch statement?

Copy link
Author

Choose a reason for hiding this comment

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

Not right now, but I want it in there for later when I'll get around to handling some of these error codes better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd delete it and re-add later it if necessary

contrib/runsim/notification.go Outdated Show resolved Hide resolved
contrib/runsim/notification.go Outdated Show resolved Hide resolved
contrib/runsim/notification.go Outdated Show resolved Hide resolved
contrib/runsim/main.go Outdated Show resolved Hide resolved
contrib/runsim/main.go Outdated Show resolved Hide resolved
contrib/runsim/main.go Outdated Show resolved Hide resolved
@mircea-c
Copy link
Author

mircea-c commented Jul 3, 2019

@alexanderbez I'm not sure what the plan is for the simulator. There's also a cosmos/tools repo that's just been added? We should maybe talk about this a bit.

As for the logs, any failed seeds will generate a message in the slack channel. The message will have the links to the log file and anyone can download them.

@codecov
Copy link

codecov bot commented Jul 3, 2019

Codecov Report

Merging #4677 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4677      +/-   ##
==========================================
+ Coverage   54.88%   54.89%   +0.01%     
==========================================
  Files         271      271              
  Lines       16975    16975              
==========================================
+ Hits         9316     9318       +2     
+ Misses       6974     6972       -2     
  Partials      685      685

mircea-c and others added 2 commits July 3, 2019 14:42
Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>
@mircea-c mircea-c requested a review from fedekunze July 3, 2019 19:19
@alessio alessio merged commit 74915f1 into master Jul 4, 2019
@alessio alessio deleted the mircea/runsim-s3-logs branch July 4, 2019 08:39
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.

5 participants