Skip to content

Maintenance: package size report failing #991

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

Closed
dreamorosi opened this issue Jun 21, 2022 · 9 comments · Fixed by #1031
Closed

Maintenance: package size report failing #991

dreamorosi opened this issue Jun 21, 2022 · 9 comments · Fixed by #1031
Assignees
Labels
automation This item relates to automation completed This item is complete and has been merged/shipped internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.)

Comments

@dreamorosi
Copy link
Contributor

Bug description

The newly merged "Package size report" step in our CI/CD is currently failing for all executions, see example here.

Expected Behavior

Step should be succeeding and report the size.

Current Behavior

It fails

Possible Solution

N/A

Steps to Reproduce

N/A

Environment

  • Powertools version used:
  • Packaging format (Layers, npm):
  • AWS Lambda function runtime:
  • Debugging logs:

Related issues, RFCs

#878

@dreamorosi dreamorosi added bug Something isn't working triage This item has not been triaged by a maintainer, please wait labels Jun 21, 2022
@dreamorosi dreamorosi added this to the production-ready-release milestone Jun 21, 2022
@flochaz
Copy link
Contributor

flochaz commented Jun 21, 2022

Dependabot only have Read-only permission (https://github.blog/changelog/2021-02-19-github-actions-workflows-triggered-by-dependabot-prs-will-run-with-read-only-permissions/) . Need to investigate what is the best to give only permission to comment (and read https://securitylab.github.com/research/github-actions-preventing-pwn-requests/)

@dreamorosi
Copy link
Contributor Author

Okay, so this issue would happen only for dependant's PRs?

For PR that come from real users would it work fine? And what about from forks?

@flochaz
Copy link
Contributor

flochaz commented Jun 22, 2022

PR from fork won't work neither.
Changing to pull_request_target with an explicit PR checkout would most likely work but is opening vulnerability (https://securitylab.github.com/research/github-actions-preventing-pwn-requests/). Risk can be mitigated with what we already have (need of approval before workflow run for forked PR) or add label check (see https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ for more details on that).

But the recommended approach would be to split in 2 flows : one to build and capture all the actions outputs in a dedicated workflow triggered on pull_request and have a chain workflow to add comment with write permission ...

@dreamorosi
Copy link
Contributor Author

Oh I see what you mean, check this PR that I was working a year ago that used data artifacts from one workflow and then used them to do something.

Thanks for looking into this!

@flochaz
Copy link
Contributor

flochaz commented Jun 22, 2022

Oh I see what you mean, check this PR that I was working a year ago that used data artifacts from one workflow and then used them to do something.

Thanks for looking into this!

But I need to change the way the action work, will propose a fix asap

@dreamorosi
Copy link
Contributor Author

yes sorry, didn't mean to say it was a solution, I was just sharing in case it was helpful

@dreamorosi dreamorosi removed this from the production-ready-release milestone Jun 23, 2022
@dreamorosi
Copy link
Contributor Author

Wanted to leave here some info to keep the issue up to date with the current state of events:

  • We have disabled Dependabot automatic PRs for regular updates in chore: disable dependabot for dependencies upgrades #992 - this means the number of Dependabot PRs will considerable decrease and be limited to security-related updates
  • For any kind of Dependabot PR I have found out that manually re-running the actions (i.e. after they have failed) removes the issue (example)
  • Considering the previous two points we currently think it's acceptable to have to manually re-run these actions given the low and sporadic amount of PRs coming from the bot
  • Alternatives such as GitHub Actions Job Summaries, paired with a threshold of the % increase that would make the action fail altogether could potentially remove the permission issue around comments

@dreamorosi dreamorosi added on-hold This item is on-hold and will be revisited in the future and removed triage This item has not been triaged by a maintainer, please wait labels Jul 7, 2022
@dreamorosi dreamorosi linked a pull request Jul 29, 2022 that will close this issue
7 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

⚠️ COMMENT VISIBILITY WARNING ⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added the pending-release This item has been merged and will be released soon label Aug 9, 2022
@dreamorosi dreamorosi removed on-hold This item is on-hold and will be revisited in the future pending-release This item has been merged and will be released soon labels Aug 9, 2022
@dreamorosi
Copy link
Contributor Author

Released as a part of v1.1.0

@dreamorosi dreamorosi changed the title Bug (build-dev): Package size report failing Maintenance: package size report failing Nov 14, 2022
@dreamorosi dreamorosi added automation This item relates to automation internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) completed This item is complete and has been merged/shipped and removed bug Something isn't working labels Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation This item relates to automation completed This item is complete and has been merged/shipped internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.)
Projects
None yet
2 participants