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

Feature/republish invoices #350

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

kiwiidb
Copy link
Contributor

@kiwiidb kiwiidb commented Apr 18, 2023

No description provided.

@kiwiidb kiwiidb requested a review from bumi April 18, 2023 09:11
@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Merging #350 (232b5b5) into main (099f63a) will decrease coverage by 1.08%.
The diff coverage is 4.54%.

@@            Coverage Diff             @@
##             main     #350      +/-   ##
==========================================
- Coverage   54.12%   53.05%   -1.08%     
==========================================
  Files          50       51       +1     
  Lines        3100     3163      +63     
==========================================
  Hits         1678     1678              
- Misses       1239     1302      +63     
  Partials      183      183              
Impacted Files Coverage Δ
republish_invoices/main.go 0.00% <0.00%> (ø)
rabbitmq/rabbitmq.go 63.80% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -50,7 +50,7 @@ func main() {
defer rabbitmqClient.Close()

result := []models.Invoice{}
err = dbConn.NewSelect().Model(&result).Where("id > ?", startId).Where("id < ?", endId).Scan(context.Background())
err = dbConn.NewSelect().Model(&result).Where("settled_at > ?", startDate).Where("settled_at < ?", endDate).Scan(context.Background())
Copy link
Contributor

Choose a reason for hiding this comment

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

we only publish settled invoices, correct? thus it's ok to check settled there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If settled_at is between those 2 values, then it's settled.

if dryRun {
continue
}
err = svc.RabbitMQClient.PublishToLndhubExchange(context.Background(), inv, svc.EncodeInvoiceWithUserLogin)
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 OK to flood the queue? or do we need some slight delay?

but I guess it should be OK.

logrus.Error(err)
}
}
logrus.Infof("Published %d invoices", len(result))
Copy link
Contributor

Choose a reason for hiding this comment

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

should we count which ones did not have an error to be able to see if all have been published.

}
err = svc.RabbitMQClient.PublishToLndhubExchange(context.Background(), inv, svc.EncodeInvoiceWithUserLogin)
if err != nil {
logrus.Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is an exception and could be a sentry notification? (though this floods sentry) - should we cancel if the publishing fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think we should cancel.

"github.com/getAlby/lndhub.go/rabbitmq"
"github.com/joho/godotenv"
"github.com/kelseyhightower/envconfig"
"github.com/sirupsen/logrus"
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we use a new logger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same logger. It's what we use under the hood.

Copy link
Contributor

Choose a reason for hiding this comment

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

how does it work? we initialize one here that we then use I think: https://github.com/getAlby/lndhub.go/blob/main/main.go#L80

rabbitmq.WithLndInvoiceExchange(c.RabbitMQLndInvoiceExchange),
rabbitmq.WithLndHubInvoiceExchange(c.RabbitMQLndhubInvoiceExchange),
rabbitmq.WithLndInvoiceConsumerQueueName(c.RabbitMQInvoiceConsumerQueueName),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a function like getRabbitmqClient to make sure we have the same client config everwhere?

@bumi
Copy link
Contributor

bumi commented Apr 18, 2023

generally I would like to move towards JSON logging which helps querying log entries.

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.

2 participants