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

Allow for use in traditional CI builds. #30

Closed
wants to merge 5 commits into from

Conversation

chabad360
Copy link
Contributor

@chabad360 chabad360 commented Nov 24, 2019

This is a PR to fix #27. It would allow for using image-actions in a push-triggred workflow.

@benschwarz
Copy link
Member

I know this is a work in progress but merging this would entirely break image-actions.

The Dockerfile that you've removed is what builds the docker container in the first place. This is not mergeable in it's current state.

@chabad360
Copy link
Contributor Author

chabad360 commented Nov 26, 2019

The Dockerfile that you've removed is what builds the docker container in the first place.

You're right, I was just trying to get it to build quickly while I was testing.

But other than that issue and the changes that are also present in my other
PR (#29), is it still breaking?
Update: This should no longer be breaking.

@chabad360 chabad360 marked this pull request as ready for review November 26, 2019 20:11
@chabad360 chabad360 changed the title [WIP] Allow for use in traditional CI builds. Allow for use in traditional CI builds. Nov 27, 2019
@chabad360
Copy link
Contributor Author

chabad360 commented Jan 14, 2020

This is now able to be merged independently of #29. Plus, it should be entierly non-breaking.

@chabad360 chabad360 mentioned this pull request Jan 14, 2020
@chabad360
Copy link
Contributor Author

It seems that the action now no longer adds a comment regarding the compression info...

@chabad360
Copy link
Contributor Author

It seems that the action now no longer adds a comment regarding the compression info...

Just realized why: it's because the image isn't being optimized enough.

@chabad360
Copy link
Contributor Author

@benschwarz can you have a look?

@benschwarz
Copy link
Member

I will. Bit snowed under atm.

@chabad360
Copy link
Contributor Author

I hate to be that guy, but just bumping this back to the notifications area...

@benschwarz
Copy link
Member

@chabad360 Could you document what this code actually does? From what I can tell, it looks for either a pull request event, or a push.

  • If it's a pull request event - Commit the files, post the comment as usual.
  • If it's a push event -- Process the images, don't commit anything, don't attempt to post a summary

How are you planning on using this? With GitHub? How would someone configure this? What would the workflow be?

@chabad360
Copy link
Contributor Author

How are you planning on using this? With GitHub?

Yes.

How would someone configure this?

Exactly the same way you do already.

What would the workflow be?

Checkout chabad360/chabad360.github.io, a single poster blog, adds image optimization to everything on build.

@kdeldycke
Copy link

kdeldycke commented Aug 4, 2020

I'd love to see that super-useful feature merged back upstream! Looks like you did a good job @chabad360 ! 😃👍

@benschwarz
Copy link
Member

This is now possible with compressOnly 👍

@benschwarz benschwarz closed this Sep 17, 2020
@kdeldycke
Copy link

Thanks @benschwarz. I was able to do what I was looking for at: kdeldycke/meta-package-manager@6f86f61

@tunetheweb
Copy link
Contributor

Hey @kdeldycke

Just so you're aware, we made the compression report available in the markdown output variable even in compressOnly mode.

So one potential improvement would be this to add an id so you can add the results of the compression in your body using {{ steps.calibre.outputs.markdown }}

So something like this:

...
      - uses: calibreapp/image-actions@1.1.0
        id: calibre
...
          body: >
            [Auto-generated on run
            #${{ github.run_id }}](https://github.com/${{ github.repository
            }}/actions/runs/${{ github.run_id }}) as defined by [workflow
            action](https://github.com/${{ github.repository
            }}/blob/${{ github.base_ref }}/.github/workflows/autofix.yaml).%0A${{ steps.calibre.outputs.markdown }}
...

Note I'm using %0A as an escaped newline to give a break between your body and the report.

You can see an example of this here.

@kdeldycke
Copy link

Oh. That's very nice! Thanks @bazzadp for the tip! 😃

kdeldycke added a commit to kdeldycke/meta-package-manager that referenced this pull request Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Compressing for builds
4 participants