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

Do not rely on the asset pipeline when calling backend PDF generation #18236

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Do not rely on the asset pipeline when calling backend PDF generation #18236

merged 1 commit into from
Dec 5, 2018

Conversation

skateman
Copy link
Member

There were some extra asset pipeline-related calls even after #18226 in the backend PDF generation code that caused problems in some special cases. I have no idea why would we call the asset_path on a file twice, but now it's not called even once.

@miq-bot add_label gaprindashvili/no, hammer/yes, bug

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650057

@miq-bot
Copy link
Member

miq-bot commented Nov 28, 2018

Checked commit skateman@1c1bd95 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@JPrause
Copy link
Member

JPrause commented Nov 29, 2018

@miq-bot add_label blocker

@skateman
Copy link
Member Author

skateman commented Dec 4, 2018

@miq-bot add_reviewer @himdel

@miq-bot miq-bot requested a review from himdel December 4, 2018 08:27
himdel
himdel approved these changes Dec 4, 2018
View reviewed changes
Copy link
Contributor

@himdel himdel left a comment

Choose a reason for hiding this comment

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

LGTM :)

Looks like AssetWriter is no longer relevant now that ImageEncodeHelper is gone and the css only includes external fonts.

@skateman
Copy link
Member Author

skateman commented Dec 4, 2018

@h-kataria or @martinpovolny can you merge please?

@martinpovolny martinpovolny merged commit ce0cd61 into ManageIQ:master Dec 5, 2018
@martinpovolny martinpovolny added this to the Sprint 101 Ending Dec 17, 2018 milestone Dec 5, 2018
@martinpovolny martinpovolny self-assigned this Dec 5, 2018
@skateman skateman deleted the asset-pipeline-pdf branch December 5, 2018 08:50
simaishi pushed a commit that referenced this pull request Dec 6, 2018
@martinpovolny @simaishi
Merge pull request #18236 from skateman/asset-pipeline-pdf
2f9fa80 
Do not rely on the asset pipeline when calling backend PDF generation

(cherry picked from commit ce0cd61)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650057
@simaishi
Copy link
Contributor

simaishi commented Dec 6, 2018

Hammer backport details:

$ git log -1
commit 2f9fa80e8513b100014bf40f4cdcd7f8a0d325e6
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Wed Dec 5 09:45:51 2018 +0100

    Merge pull request #18236 from skateman/asset-pipeline-pdf
    
    Do not rely on the asset pipeline when calling backend PDF generation
    
    (cherry picked from commit ce0cd617bd1a98a9f8d266529d01e878231528ef)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650057

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himdel himdel himdel approved these changes

Assignees

@martinpovolny martinpovolny

Labels
blocker bug hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@skateman @miq-bot @JPrause @simaishi @himdel @martinpovolny