axel-op / dart-package-analyzer

GitHub Action that uses the Dart Package Analyzer to compute the Pub score of Dart/Flutter packages
MIT License
52 stars 9 forks source link

Use image on dockerhub instead of a Dockerfile #1

Closed vaind closed 5 years ago

vaind commented 5 years ago

Is there any reason the action is set up to build an image each time? It takes 2-3 minutes on each run which could be avoided if the action has used a pre-built image.

from a pipeline:

Build axel-op/dart_package_analyzer@stable2m 47s
+ http 0.12.0+2
Build container for action use: '/home/runner/work/_actions/axel-op/dart_package_analyzer/stable/Dockerfile'.
/usr/bin/docker build -t e959fb:8ba3abdaf5da4bc49bdc5ac1a36bc06a "/home/runner/work/_actions/axel-op/dart_package_analyzer/stable"

If I'm not mistaken, this is the line that would need to be changed (besides pushing the image to dockerhub): https://github.com/axel-op/dart_package_analyzer/blob/stable/action.yml#L21

What do you think?

axel-op commented 5 years ago

Hi,

Thanks for the suggestion! I'm pretty new to Docker, that's why I haven't thought about that. However, creating an image would mean to freeze the Flutter version used, instead of always having the latest. Do you have an idea about how I could handle that?

Also, I looked into the comments of the PR you linked.

From https://github.com/objectbox/objectbox-dart/pull/31#issuecomment-540137407:

This line basically: githubToken: ${{ secrets.GITHUB_TOKEN }}. I cannot set that secret up and wanted to know if GITHUB_TOKEN is fine as a key or if a different key is better. Maybe PANA_GITHUB_TOKEN so you know that it's about package analysis?

@GregorySech You don't have to create this secret by yourself, it is already included by GitHub in the action's context (see https://help.github.com/en/articles/contexts-and-expression-syntax-for-github-actions#github-context). You just have to write it like it is in the README.

Also you don't have to run pub get by yourselves, as my action already does that for you.

Finally I noticed you run this action twice for two different packages, but in the same job. Maybe I could recommend you to do it in two separate jobs (that you can write in the same workflow file), as each job is run in parallel by GitHub (see https://help.github.com/en/articles/workflow-syntax-for-github-actions#jobs).

vaind commented 5 years ago

Hi,

Thanks for the suggestion! I'm pretty new to Docker, that's why I haven't thought about that. However, creating an image would mean to freeze the Flutter version used, instead of always having the latest. Do you have an idea about how I could handle that?

Yes, the version would be fixed to the one you've pushed your image with, however you could even set up your CI (in this project) to update the periodically (e.g. every day) running the action on schedule.

Also, I looked into the comments of the PR you linked.

Thanks for that :)

From objectbox/objectbox-dart#31 (comment):

This line basically: githubToken: ${{ secrets.GITHUB_TOKEN }}. I cannot set that secret up and wanted to know if GITHUB_TOKEN is fine as a key or if a different key is better. Maybe PANA_GITHUB_TOKEN so you know that it's about package analysis?

@GregorySech You don't have to create this secret by yourself, it is already included by GitHub in the action's context (see https://help.github.com/en/articles/contexts-and-expression-syntax-for-github-actions#github-context). You just have to write it like it is in the README.

Also you don't have to run pub get by yourselves, as my action already does that for you.

Finally I noticed you run this action twice for two different packages, but in the same job. Maybe I could recommend you to do it in two separate jobs (that you can write in the same workflow file), as each job is run in parallel by GitHub (see https://help.github.com/en/articles/workflow-syntax-for-github-actions#jobs).

Running the action twice in the same job = trying to avoid building the image twice, though yes, I didn't realize github runs it in parallel so it was actually faster as it originally was.

GregorySech commented 5 years ago

Thanks, I'll remove the pub get ~and fix the key~ once I'll identify what differs from my local installation of pana.

edit: the key name already matches

axel-op commented 5 years ago

I've made it! Build time has been reduced threefold. I could still improve it however, I'll work on further improvements.

axel-op commented 5 years ago

Changes made!