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

i can’t get no satisfaction #107

Merged
merged 17 commits into from
Nov 12, 2018
Merged

i can’t get no satisfaction #107

merged 17 commits into from
Nov 12, 2018

Conversation

squishykid
Copy link
Member

@squishykid squishykid commented Nov 7, 2018 •
edited

When we get a new image in the bucket:

  • Check the filename is ok
  • Get a url for the file
  • Send to Azure and get the details
  • Apply the dimensional reduction
  • Save the reduced data in the database

@squishykid squishykid self-assigned this Nov 7, 2018
@squishykid squishykid requested review from a team as code owners November 7, 2018 04:55
@squishykid squishykid mentioned this pull request Nov 7, 2018
Closed
4 tasks
@arosspope
Copy link
Member

Also looks good, will you bother with the database connection in this PR? If not, perhaps it'd be a good idea to make a comment that shows inserting the customer satisfaction to the database.

@squishykid
Copy link
Member Author

Planning to do the database connection too

@squishykid squishykid added the work in progress Work in Progress label Nov 7, 2018
@squishykid squishykid added this to the End of Build 3 milestone Nov 7, 2018
@squishykid
Copy link
Member Author

squishykid commented Nov 11, 2018 •
edited

Still gotta do a smoke test and then this will be ready to review.

Update: I'll do the smoke test later, need to get another Azure key

m-sachi
m-sachi requested changes Nov 11, 2018
View reviewed changes
Copy link

@m-sachi m-sachi left a comment

Choose a reason for hiding this comment

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

comment comment comment

biz/css/emotion_recognition.py Show resolved Hide resolved
biz/css/emotion_recognition.py Show resolved Hide resolved
events/satisfaction_lambda.py Show resolved Hide resolved
events/satisfaction_lambda.py Show resolved Hide resolved
events/satisfaction_lambda.py Outdated Show resolved Hide resolved
m-sachi
m-sachi previously approved these changes Nov 11, 2018
View reviewed changes
@squishykid squishykid added review please Please review and removed work in progress Work in Progress labels Nov 11, 2018
@squishykid
Copy link
Member Author

Waiting for technical review from @arosspope

arosspope
arosspope reviewed Nov 11, 2018
View reviewed changes
Copy link
Member

@arosspope arosspope left a comment

Choose a reason for hiding this comment

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

Just a question about committing the db transaction.

events/satisfaction_lambda.py Show resolved Hide resolved
@arosspope
Copy link
Member

Other than that one question, this looks good to merge.

arosspope
arosspope previously approved these changes Nov 12, 2018
View reviewed changes
Copy link
Member

@arosspope arosspope left a comment

Choose a reason for hiding this comment

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

LGTM

@squishykid squishykid added ready to merge ready to merge deliverable and removed review please Please review ready to merge ready to merge labels Nov 12, 2018
@squishykid squishykid merged commit 4d5e7c5 into master Nov 12, 2018
@squishykid squishykid deleted the cant-get-satisfaction branch November 12, 2018 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arosspope arosspope arosspope approved these changes

@m-sachi m-sachi m-sachi left review comments

Assignees

@squishykid squishykid

Labels
deliverable
Projects
None yet
Milestone
End of Build 3
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@squishykid @arosspope @m-sachi