Conversation
bramkragten
left a comment
There was a problem hiding this comment.
LGTM, but no github action expert
ludeeus
left a comment
There was a problem hiding this comment.
Needs an adjustment
However, I don't think we should add this workaround as it goes against our ADR (0012)
https://github.com/home-assistant/architecture/blob/master/adr/0012-define-supported-installation-method.md#decision
We are open for contributions that improve compatibility with a community-supported method as long as they do not impact officially supported methods, add a significant amount of code exceptions or future maintenance burden on the Home Assistant development team.
This will additional maintenance we do now have for our supported installation methods.
I would argue that the maintenance requirements are quite low once it's up and running. Furthermore, the original plan was to upload both sdists and wheels to PyPI. The only reason we had to change that plan were space constraints, it just adds up quickly to upload twice the amount of data each time. Lastly, I'll provide a good archive storage for us. We needed to delete some releases from PyPI recently due to the before mentioned space constraints. If we would want to upload them again once the limit is increased, we would have to build each tag from scratch. That's just unfeasible. |
|
I've created a composite action which would help simplify the changes here. It would also allow us to reuse it in |
balloob
left a comment
There was a problem hiding this comment.
I want to move forward with this PR.
Reasoning is that translations are outputted during build time. By only storing them on PyPI inside a wheel, we don't have a single source of truth of the package.
This PR would solve that.
|
Tested the change again, against my own fork. Everything worked as expected, so this PR could be merged. |
Co-authored-by: Joakim Sørensen <hi@ludeeus.dev>
|
Thanks @frenck for the suggestion! I've replace the existing code to use the Example log output |
| permissions: | ||
| contents: write # Required to upload release assets |
There was a problem hiding this comment.
| permissions: | |
| contents: write # Required to upload release assets |
It already have this?
https://github.com/home-assistant/frontend/runs/5578172811?check_suite_focus=true#step:1:16
There was a problem hiding this comment.
It's recommended to limit the permissions to what's necessary.
https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token
As a good security practice, you should grant the GITHUB_TOKEN the least required access.
By adding only contents: write explicitly, I exclude all other permission scopes.
There was a problem hiding this comment.
If that is the reason behind it, it should be defined on the workflow as read, and then open each job to what they need.
There was a problem hiding this comment.
31cc0a1 I checked the other jobs. None need additional permissions.
I wasn't sure about artifact upload / download, but after testing it seems to work just fine.
For the future, it might be worth considering to limit the default repo1 / org2 permissions for Github tokens.
With regards to the other workflows, only a few would need explicit permissions.
release-drafter->contents: writeto create pre-releaseslock3 andstale4 ->issues: writeandpull-requests: write
Footnotes
-
https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#setting-the-permissions-of-the-github_token-for-your-repository ↩
-
https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization#setting-the-permissions-of-the-github_token-for-your-organization ↩
-
https://github.com/dessant/lock-threads#examples ↩
-
https://github.com/actions/stale#recommended-permissions ↩
There was a problem hiding this comment.
Uploading action artifacts needed "deployment" before, but if it works without anything that's just great 👍
As for changing the default of the entire org, I 100% agree with that, both in regards to the token, and potentially implementing an allow list for approved actions.
|
Just to make sure, could someone check that third party actions are allowed in the repository settings? Or at least add |
|
@cdce8p Works. |
Proposed change
Add CI job to upload
sdistandwheelas release assets. This will provide an alternative for the users who depended on thesdistwhich is no longer uploaded to PyPI. All officially supported install methods work fine with the provided wheel.I've tested the workflow on my personal fork extensively, so I'm confident that it should work. To know for certain though, we'll have to wait for the next release.
Fixes: #11542
/CC: @bramkragten @frenck
Depends on
release-assetsaction actions#61Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: