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: add barcode extension #459

Merged
merged 6 commits into from
Jul 2, 2024
Merged

Conversation

jeromeag
Copy link
Contributor

@dduportal dduportal changed the title Feature/barcode feature: add barcode extension Jul 2, 2024
Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Hi @jeromeag , many thanks for this feature. It looks good and legit!

Before approving it, there are a few steps left:

  • Could you give us a size (approximatively) difference before and after the change (to evaluate the change impact)?
  • Could you update your change to use fixed version to the 3 new Gems (following other examples) so it would be documented by the automation, and I could add the version tracking with updatecli?

@jeromeag
Copy link
Contributor Author

jeromeag commented Jul 2, 2024

Hi @dduportal,
Thank you for your quick feedback !

Hi @jeromeag , many thanks for this feature. It looks good and legit!

Before approving it, there are a few steps left:

* Could you give us a size (approximatively) difference before and after the change (to evaluate the change impact)?

Here is the before/after results of command docker image ls after make build-load

Before:

$ docker image ls 
REPOSITORY    TAG       IMAGE ID       CREATED       SIZE
asciidoctor   latest    2de7ba181c26   4 hours ago   791MB
<none>        <none>    95010f29fbfd   4 days ago    47MB
<none>        <none>    794e485ffc6e   3 weeks ago   325MB

After:

REPOSITORY    TAG       IMAGE ID       CREATED          SIZE
asciidoctor   latest    c478415b410d   28 seconds ago   793MB
<none>        <none>    2de7ba181c26   4 hours ago      791MB
<none>        <none>    95010f29fbfd   4 days ago       47MB
<none>        <none>    794e485ffc6e   3 weeks ago      325MB
* Could you update your change to use fixed version to the 3 new Gems (following other examples) so it would be documented by the automation, and I could add the version tracking with `updatecli`?

Done !

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Nice job, thanks!

  • I'm approving this PR and setting up to auto-merge (merging only if checks are passing but the code looks good!)
  • This change only adds 2 Mb to the image: it's fine (image size is close to ~390 Mb).
  • Once merged, a new release of the asciidoctor Docker image will be done to provide this change (and the gnuplot one) through a versioned release
  • Subsequent PR will be created (I'll take care of it unless you are interested to do it of course) to add version tracking automation (so further Gem updates on these 3 news will end with an automated bump PR ala Dependanbot)

@dduportal dduportal enabled auto-merge July 2, 2024 13:20
@dduportal dduportal merged commit 56c9097 into asciidoctor:main Jul 2, 2024
2 checks passed
@jeromeag jeromeag deleted the feature/barcode branch July 10, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants