Skip to content

release-assets action followup#67

Closed
cdce8p wants to merge 1 commit intohome-assistant:masterfrom
cdce8p:release-assets-followup
Closed

release-assets action followup#67
cdce8p wants to merge 1 commit intohome-assistant:masterfrom
cdce8p:release-assets-followup

Conversation

@cdce8p
Copy link
Copy Markdown
Member

@cdce8p cdce8p commented Mar 15, 2022

@cdce8p cdce8p marked this pull request as draft March 15, 2022 15:15
@cdce8p cdce8p marked this pull request as ready for review March 15, 2022 18:55
@cdce8p cdce8p marked this pull request as draft March 15, 2022 19:02
@cdce8p cdce8p marked this pull request as ready for review March 15, 2022 19:41
@cdce8p cdce8p requested a review from ludeeus March 15, 2022 19:42
Copy link
Copy Markdown
Member

@ludeeus ludeeus left a comment

Choose a reason for hiding this comment

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

Thanks!
Another note about this action (not related to this PR, but something we should consider), this is not that reusable, as this will handle wheels and sdist for python packages only.

I think it's better to refactor this action to take a single file (or all files in the folder passed), so it can be used in other types of repos like https://github.com/home-assistant/operating-system/releases/tag/7.5

And potentially split it up into 2 actions:

  • Get current assets (helpers/release-assets/list)
  • Upload asset(s) (helpers/release-assets/upload)

That way the workflow that calls the action can skip build steps if the asset already exists.

@cdce8p
Copy link
Copy Markdown
Member Author

cdce8p commented Mar 16, 2022

Thanks! Another note about this action (not related to this PR, but something we should consider), this is not that reusable, as this will handle wheels and sdist for python packages only.

I think it's better to refactor this action to take a single file (or all files in the folder passed), so it can be used in other types of repos like https://github.com/home-assistant/operating-system/releases/tag/7.5

And potentially split it up into 2 actions:

  • Get current assets (helpers/release-assets/list)
  • Upload asset(s) (helpers/release-assets/upload)

That way the workflow that calls the action can skip build steps if the asset already exists.

I've started workin on that. Won't finish it until later today though.
Atm I've created two sub-actions: list and upload-file. I'm a bit skeptical about uploading a whole folder. At least here it wouldn't quite work as both files have different content-types application/zip vs application/gzip.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 16, 2022

Does the upload action require to set the right content type?

@cdce8p
Copy link
Copy Markdown
Member Author

cdce8p commented Mar 16, 2022

Does the upload action require to set the right content type?

I think so. https://docs.github.com/en/rest/reference/releases#upload-a-release-asset

[...]
Use the required Content-Type header to provide the media type of the asset.
For a list of media types, see [Media Types](https://www.iana.org/assignments/media-types/media-types.xhtml).
For example: application/zip

--
Just doing some final testing, but the next version will be much more versatile. It can can handle uploading an entire folder or individual files and includes a configurable mapping between file names and content types.
master...cdce8p:rewrite-release-assets

Example job step for the frontend action

      - name: Upload release assets
        uses: home-assistant/actions/helpers/release-assets@master
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
          path: dist/
          file_filter: "*.whl, *.tar.gz"
          content_type_map: "*.whl=application/zip, *.tar.gz=application/gzip"
          delete_existing_assets: true

I'll open a new actions PR for it later.

@balloob
Copy link
Copy Markdown
Member

balloob commented Mar 16, 2022

Nice!

Could you derive the file filter from the content map?

@cdce8p
Copy link
Copy Markdown
Member Author

cdce8p commented Mar 16, 2022

Could you derive the file filter from the content map?

Great idea! I've also change path to paths which now allows passing multiple comma-separated values.
Opened #68 with the rewrite PR.

@cdce8p cdce8p marked this pull request as draft March 16, 2022 23:43
@cdce8p
Copy link
Copy Markdown
Member Author

cdce8p commented Mar 23, 2022

No longer needed. See #69

@cdce8p cdce8p closed this Mar 23, 2022
@cdce8p cdce8p deleted the release-assets-followup branch March 23, 2022 03:57
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.

4 participants