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

build: update CI step for creating changesets + reporting coverage #2305

Merged
merged 6 commits into from
May 27, 2024

Conversation

maschad
Copy link
Member

@maschad maschad commented May 14, 2024

This also relocates the create-changesets job to the PR-lint step so that in the case where dependabot does open the PR, that step is run before it's linted.

Closes #2320

@petertonysmith94
Copy link
Contributor

petertonysmith94 commented May 15, 2024

  • As the test (or report-coverage) is a required step in our pipeline - I'm not entirely convinced this will work for dependabot, unless we make the coverage report not required.
  • I believe the validate-changeset job would need to be dependant on create-changeset job for this to be reliable.
  • The create-changeset job will require elevated permissions to write the commit.

I'd personally revert and re-evaluate our overall strategy.

There are security considerations to be had when elevating permissions and we should understands the risks fully before doing so IMO.

to prevent potentially compromised dependencies from capturing secrets referenced in your workflows.

@maschad
Copy link
Member Author

maschad commented May 15, 2024

  • As the test (or report-coverage) is a required step in our pipeline - I'm not entirely convinced this will work for dependabot, unless we make the coverage report not required.

The rationale here is that we don't need to report coverage for dependabot PRs given an upgrade in a dependency would not result in a difference in the coverage, there is nothing in the workflows to suggest that the test/report-coverage step is required.

  • I believe the validate-changeset job would need to be dependant on create-changeset job for this to be reliable.
  • The create-changeset job will require elevated permissions to write the commit.

Consider the two branches:

  1. if the PR is not created by dependabot, then the create-changeset job will not be run, and thus validate-changeset will only fail if the creator of that PR has not added a changeset, which is the desired outcome.

  2. if the PR was created by dependabot, then create-changeset job runs, if it fails, the validate-changeset will fail, this is desirable because there would be a need for a changeset, otherwise the validate-changeset passes and all is well.

I'd personally revert and re-evaluate our overall strategy.

There are security considerations to be had when elevating permissions and we should understands the risks fully before doing so IMO.

to prevent potentially compromised dependencies from capturing secrets referenced in your workflows.

My understanding when adding the create-changeset job was that no elevated permissions were given to dependabot in as the commit is done by the github bot and not dependabot, but that could be wrong. We could consider using the @fuel-service-user to create the changesets for dependency upgrades if this is a concern, otherwise we could forgo changesets for dependency upgrades.

Interested to hear others thoughts @nedsalk @Torres-ssf @arboleya @Dhaiwat10 @danielbate

@danielbate
Copy link
Contributor

there is nothing in the workflows to suggest that the test/report-coverage step is required.

This is set in the repository settings rather than in the workflow. @arboleya can set this / change it to report-coverage if we wanted. But test is currently a required stage.

I think we should be adding changesets for dependabot PRs, should these introduce issues it'll make them easier to patch.

My concern is that if the perms were not correct for coverage reporting, I'd assume changeset creation would be similar. Testing @fuel-service-user is a good move but we should do this sooner rather than later, and if it proves unsuccessful we should revert #2295 and test in isolation.

@arboleya
Copy link
Member

arboleya commented May 16, 2024

@maschad I won't be able to follow up on this soon. I suggest you guys do huddle/call about it to be more productive and reach a consensus. If the revert is the solution for now, so be it. Otherwise, whatever fixes the problem, fix the problem. We need to get those workflows green again.

cc @petertonysmith94 @nedsalk @Torres-ssf @danielbate

@maschad maschad marked this pull request as draft May 16, 2024 17:48
@maschad maschad force-pushed the mc/ci/update-coverage-workflow branch from 45834ea to 2eb7bbb Compare May 16, 2024 19:43
@maschad maschad force-pushed the mc/ci/update-coverage-workflow branch from a06e486 to 91b881f Compare May 20, 2024 20:14
@maschad
Copy link
Member Author

maschad commented May 20, 2024

I realize that we may have overcomplicated things. We can simply change the workflow to have write permissions and then only run this job when it is dependabot acting , I took inspiration from the common dependabot operations

@maschad maschad marked this pull request as ready for review May 20, 2024 20:14
Copy link
Contributor

@danielbate danielbate left a comment

Choose a reason for hiding this comment

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

Has this been tested on a fork?

@maschad
Copy link
Member Author

maschad commented May 21, 2024

Has this been tested on a fork?

Unfortunately that's not possible (with good reason) as you can imagine the implications of allowing someone who doesn't have write permissions to a repository but could configure GitHub Actions to respond to do so.

Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

Happy to approve 🍏 Can't see anything that could block the CI like the previous iterations.

Still skeptical if this will work, as I believe dependabot will not be able to request the permissions that are required (akin to externals forks).

@maschad
Copy link
Member Author

maschad commented May 23, 2024

Still skeptical if this will work, as I believe dependabot will not be able to request the permissions that are required (akin to externals forks).

@petertonysmith94 that's valid. It's not exactly clear in the Github documentation, suggests we can use the permissions key in our workflow to increase the access for the GITHUB_TOKEN. What we can do to be more certain is add a dependabot secret as you had suggested to be certain.

@maschad maschad enabled auto-merge (squash) May 23, 2024 22:37
@maschad maschad disabled auto-merge May 23, 2024 22:37
@maschad maschad enabled auto-merge (squash) May 27, 2024 17:47
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
46.37%(+0%) 39.55%(+0%) 43.6%(+0%) 46.2%(+0%)
Changed Files:

Coverage values did not change👌.

@maschad maschad merged commit 72589fa into master May 27, 2024
19 checks passed
@maschad maschad deleted the mc/ci/update-coverage-workflow branch May 27, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: fix Dependabot workflow for automating changesets
5 participants