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

Image comparison: use source url #129

Open
wants to merge 1 commit into
base: entities
Choose a base branch
from
Open

Conversation

cdworak
Copy link
Contributor

@cdworak cdworak commented May 20, 2019

No description provided.

@cdworak cdworak requested review from bhainesva and trob-indie May 20, 2019 15:46
Copy link
Member

@mhupman mhupman left a comment

Choose a reason for hiding this comment

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

Actually I'm not so sure this is desirable - this only works if the source image urls change every time there is a change to the image. In an ETL like ------- Headshot, this wouldn't work because their URLs don't change even if the underlying images do.

func (a *Image) Equal(b Comparable) bool {
defer func() {
if r := recover(); r != nil {
log.Printf("Value of A: %+v, Value of B:%+v, Type Of A: %T, Type Of B: %T\n", a, b, a, b)
Copy link
Member

Choose a reason for hiding this comment

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

mean to leave this in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it follows the model of the other Equal implementations in yext-go

}()

if a.SourceUrl != nil {
a.Url = a.SourceUrl
Copy link
Member

Choose a reason for hiding this comment

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

mutating the source seems wacky - can we compare the source URL directly?

Copy link
Contributor Author

@cdworak cdworak May 20, 2019

Choose a reason for hiding this comment

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

sure but i wanted to make sure it was only the source url and url that is different, i don't have a great way of doing that other than porting reflection here (the diff) or writing code to compare attribute to attribute, which if we add an attribute we'd have to remember to update this code too, so that seems dangerous

@cdworak
Copy link
Contributor Author

cdworak commented May 20, 2019

I think if ------- doesn't update their source than we should probably add a vparam to the end of the URL with today's date and we'd essentially have no change of functionality.

For clients that do update their source we get duplicate updates, which messes with stats and fills the db unnecessarily, so I'd prefer the vparam route if possible.

}()

if a.SourceUrl != nil {
a.Url = a.SourceUrl
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm uncomfortable with the idea of the equal function modifying the receiver that it's called on? could we copy it instead and then call genericdiff on the copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure!

@mhupman
Copy link
Member

mhupman commented May 20, 2019

I think if -------- doesn't update their source than we should probably add a vparam to the end of the URL with today's date and we'd essentially have no change of functionality.

For clients that do update their source we get duplicate updates, which messes with stats and fills the db unnecessarily, so I'd prefer the vparam route if possible.

You mean modify their source URL? How so?

@bhainesva
Copy link
Contributor

bhainesva commented May 20, 2019

Just want to add another use case, ---- also has an etl that updates images without updating the url

@mhupman
Copy link
Member

mhupman commented May 20, 2019

We could make this opt-in (or out) by using a property on the image itself? e.g. image.SetUnpredictableImageContents(true)

@benmcginnis
Copy link
Member

Another potential avenue would be to add a hash property on the image itself and use that for diffing if it's populated. That would allow us to have calling code handle it by downloading the image and creating a hash of the contents to diff against for cases where we don't get changed URLs and when we do get changed URLs we can just set the hash to the url itself.

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.

4 participants