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

upgrade xhtml2pdf #21559

Merged
merged 3 commits into from
Aug 17, 2018
Merged

upgrade xhtml2pdf #21559

merged 3 commits into from
Aug 17, 2018

Conversation

nickpell
Copy link
Contributor

@nickpell nickpell added the product/invisible Change has no end-user visible impact label Aug 16, 2018
@nickpell nickpell requested review from calellowitz and emord August 16, 2018 02:56
@nickpell
Copy link
Contributor Author

@calellowitz @emord Is this something that's easy to test after deploy?

@emord
Copy link
Contributor

emord commented Aug 16, 2018

I think that you'll need to run the make file to actually update the requirements

@emord
Copy link
Contributor

emord commented Aug 16, 2018

yeah. should be easy to test on india

@nickpell nickpell force-pushed the np/upgrade-xhtml2pdf branch from e0ec80e to 74d4788 Compare August 16, 2018 21:30
@nickpell
Copy link
Contributor Author

I think that you'll need to run the make file to actually update the requirements

woops, fixed

@nickpell
Copy link
Contributor Author

yeah. should be easy to test on india

Cool - I just tested downloading the generated pdf from india. My plan is to merge this branch, deploy, test, and then give the go-ahead to merge to NIC.

@@ -13,6 +13,7 @@ alembic==0.8.6
amqp==1.4.9
anyjson==0.3.3
appdirs==1.4.3
appnope==0.1.0 # via ipython
Copy link
Contributor

Choose a reason for hiding this comment

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

confused by this line. #21563

not sure if it's necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"The Internet" says that appnope is a dependency of ipython on OSX, but not Linux. Probably #21563 was generated in a Linux environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

This also changes prod-requirements.txt. Since our servers run linux, I think I'd default to whatever is generated.

With that said, from the linked ticket, it doesn't seem like there's a way to enforce this consistently. I'll create an issue and ping danny on it, to see if he knows more after he gets back from offline time

@@ -13,6 +13,7 @@ alembic==0.8.6
amqp==1.4.9
anyjson==0.3.3
appdirs==1.4.3
appnope==0.1.0 # via ipython
Copy link
Contributor

Choose a reason for hiding this comment

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

This also changes prod-requirements.txt. Since our servers run linux, I think I'd default to whatever is generated.

With that said, from the linked ticket, it doesn't seem like there's a way to enforce this consistently. I'll create an issue and ping danny on it, to see if he knows more after he gets back from offline time

@nickpell nickpell merged commit e677536 into master Aug 17, 2018
@nickpell nickpell deleted the np/upgrade-xhtml2pdf branch August 17, 2018 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants