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

chore: add chromium to docker for html-template #437

Merged
merged 2 commits into from
Oct 30, 2020
Merged

chore: add chromium to docker for html-template #437

merged 2 commits into from
Oct 30, 2020

Conversation

derberg
Copy link
Member

@derberg derberg commented Oct 28, 2020

Description

  • add chromium to docker for html-template

Related issue(s)
Fixes asyncapi/html-template#112

@derberg derberg requested a review from fmvilas October 28, 2020 16:38
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

@derberg is it not possible to get around adding a global package to the dockerfile to accommodate 1 template? It seems... Wrong 😄

@derberg
Copy link
Member Author

derberg commented Oct 29, 2020

@jonaslagoni yeah, I know what you mean, but there is no other way really. At the end, html-template is an official template, so I do not see this as a big problem.

@jonaslagoni
Copy link
Member

jonaslagoni commented Oct 29, 2020

@jonaslagoni yeah, I know what you mean, but there is no other way really. At the end, html-template is an official template, so I do not see this as a big problem.

@derberg can't we imagine this is something everyone could benefit from, adding a feature which controls the docker environment per template? I can understand if we just want to make this a temporary fix, however there should be an issue where this is raised IMO.

@derberg
Copy link
Member Author

derberg commented Oct 29, 2020

@jonaslagoni tbh I have no idea how else this could be solved, other than saying create your own image using generator image as a base. I also cannot really imagine any other template needing such a custom approach. Puppeteer is just unique here needing dedicated chromium instance for alpine. What I'm trying to say is that any followup issue will just be food for the stale bot 😄 as we do not have a use case for such extensibility. Maybe I add a comment that in case more customizations are needed for other templates, we should consider reworking the image?

@jonaslagoni
Copy link
Member

What I'm trying to say is that any followup issue will just be food for the stale bot 😄 as we do not have a use case for such extensibility. Maybe I add a comment that in case more customizations are needed for other templates, we should consider reworking the image?

I mean, you just had the use case 😄 If 1 template requires custom dependencies, others will probably too, even if we can't think of a reason atm. I mean there are different methods to make it require dependencies, extending the generator docker image, make a framework for shell files, make files etc. But I agree atm this is too much, but I still think an issue would be valid. Ill accept the PR on the notion that this is temporary 😄

jonaslagoni
jonaslagoni previously approved these changes Oct 30, 2020
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

see comment.

@derberg
Copy link
Member Author

derberg commented Oct 30, 2020

@jonaslagoni yo, can you approve again, I added a comment to dockerfile

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg derberg merged commit 38737fe into asyncapi:master Oct 30, 2020
@derberg derberg deleted the fix-chromium branch October 30, 2020 09:22
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.

Crash when generating pdf
2 participants