Skip to content

Dependabot Enhancements #715

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

Conversation

ChristianZaccaria
Copy link
Collaborator

@ChristianZaccaria ChristianZaccaria commented Oct 18, 2024

Issue link

Jira: https://issues.redhat.com/browse/RHOAIENG-11457

What changes have been made

Note: in this repository I have enabled merge queues in the main branch + in a dummy branch for testing purposes.

  • Ignore updating patch versions to focus on major and minor updates.
  • Limit number of PRs opened.
  • Add dependabot labeler workflow to add required labels to automatically add PRs to merge queues.
  • Add merge_group condition on test workflows to be ran on merge queues.

Verification steps

Merge queues:

  1. I added changes from this PR to a dummy branch.
  2. I created Dummy PR 1: First Dummy PR to test in Merge Queue #716
  3. I created Dummy PR 2: Second Dummy PR to test in Merge Queue #717
  4. See both PRs were added to the merge queue, and both were successfully merged at the exact same time.

Dependabot:

  1. PRs generated in my fork for:
  • pyproject.toml and poetry.lock files.
  • requirements.txt files.
  • package.json and yarn.lock files.

Once this PR is merged, we can make use of the CodeFlare Machine token to verify the Dependabot-Labeler workflow adds the required labels to add the Dependabot PRs to the merge queue.

Points for discussion:

Before we merge, I would like to discuss with the team if we should perhaps change the way we merge PRs. To significantly reduce costs of using expensive GitHub Runners such as the ones with GPUs ran in e2e tests, perhaps the new flow could be:

If PR passes all tests including pre-commit, unit-tests, and code-coverage (low costs), then the PR is added to the merge queue where the e2e test will run with changes from this PR and the number of all other PRs in the merge queue.

If e2e fails, the PR is removed from the merge queue, if passes, the PR is merged with all other PRs in the merge queue.

This would mean, e2e tests will not run on each opened PR, or on each push, significantly reducing the running costs.

Note: we could still allow to run e2e tests in a PR triggered by a non-merge-queue label perhaps.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

@ChristianZaccaria ChristianZaccaria added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.11%. Comparing base (3c0008d) to head (1c05902).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #715   +/-   ##
=======================================
  Coverage   94.11%   94.11%           
=======================================
  Files          36       36           
  Lines        2412     2412           
=======================================
  Hits         2270     2270           
  Misses        142      142           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024
@ChristianZaccaria
Copy link
Collaborator Author

PR ready for merge. - One thing to test here, if an approver adds the lgtm and approved labels, will this PR be sent to the merge queue or merged directly. There 'may' be conflicting processes between openshift prow and github merge queues. However, the default process is to Merge when ready.

@ChristianZaccaria ChristianZaccaria force-pushed the dependabot-enhancement branch 2 times, most recently from a0a0bae to 9589eb3 Compare October 22, 2024 10:25
@ChristianZaccaria ChristianZaccaria added e2e and removed e2e labels Oct 22, 2024
Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

Looks good! This should keeps PR tab much cleaner

@ChristianZaccaria ChristianZaccaria force-pushed the dependabot-enhancement branch 2 times, most recently from 5273ced to 6e61a47 Compare October 22, 2024 16:05
- Ignore updating patch versions to focus on major and minor updates.
- Limit number of PRs opened.
- Add dependabot labeler workflow to add required labels to automatically add PRs to merge queues.
- Add merge_group condition on test workflows to be ran on merge queues.
Copy link
Collaborator

@KPostOffice KPostOffice left a comment

Choose a reason for hiding this comment

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

A quick question about the e2e tests being run on all PRs in the merge queue. If this does fail, how does GitHub decide which PRs to back out of the merge queue? Can we run e2e specifically once a test has been backed out of the queue?

@Bobbins228
Copy link
Contributor

Once this PR is merged, we can make use of the CodeFlare Machine token to verify the Dependabot-Labeler workflow adds the required labels to add the Dependabot PRs to the merge queue.

@ChristianZaccaria you can use your own PAT as the CodeFlare Machine Account Token for testing on your fork.

@ChristianZaccaria
Copy link
Collaborator Author

ChristianZaccaria commented Oct 23, 2024

@KPostOffice The flow of GitHub Merge queues goes like this:
For this example assume all following PRs are ready to merge.

  1. We add PR-1 to the merge queue -> a new temporary branch is automatically created with changes from PR-1 and target branch -> e2e is ran against it.
  2. While the e2e is running on PR-1, we add PR-2 to the merge queue before PR-1 e2e test passes -> a new new temp branch is created with changes from PR-2, PR-1, and the target branch. -> e2e tests run here now.

A few scenarios can happen at this point:

  • Tests passed on the temp branch with {PR-2 + PR-1 + target branch}, then both PRs get merged at the same time.
  • In a scenario where PR-1 e2e failed but passed when running with {PR-2 + PR-1 + target branch}, GitHub merge queues merges both as it proved they pass the required tests together.
  • Another scenario, PR-1 fails e2e, and {PR-1 + PR-2 + target branch} fail e2e, GitHub Merge Queues will drop PR-1 from the queue and have PR-2 retested. If PR-2 passes on its own, it gets merged. For PR-1, we can then run e2e individually in the PR by adding the e2e label.

Reference

@ChristianZaccaria ChristianZaccaria force-pushed the dependabot-enhancement branch 2 times, most recently from 02813fb to 5e81252 Compare October 23, 2024 10:33
@ChristianZaccaria
Copy link
Collaborator Author

@Bobbins228 merge queues is a feature that can only be enabled in an org, so I couldn't test this in my own fork.

I tried to test the dependabot labeler workflow in a dummy branch in the codeflare-sdk, but it seems unless it is merged in main, the workflow won't have access to the required secrets in the org. - Not sure how to proceed

@Bobbins228
Copy link
Contributor

You can merge the branch into your own fork to access the labeller workflow 👍

@ChristianZaccaria
Copy link
Collaborator Author

Here is a dummy PR used to test the dependabot labeler in my fork: ChristianZaccaria#182 - works as expected.

@Bobbins228
Copy link
Contributor

Should the guided demo nbs label have been added to that PR? @ChristianZaccaria

@Bobbins228
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2024
@ChristianZaccaria
Copy link
Collaborator Author

There are 2 items that can be tested during merge:

  • lgtm and approved labels automatically send the PR to merge queue.
  • PR in merge queue runs through e2e test.

Item to test after merge:

  • Check newly created Dependabot PRs. - Check if all labels are added, e2e test is ran in merge queue, and PR gets merged automatically.

@KPostOffice
Copy link
Collaborator

@KPostOffice The flow of GitHub Merge queues goes like this: For this example assume all following PRs are ready to merge.

...

@ChristianZaccaria Thanks! Appreciate the explanation. I think I'm good to approve this then and we can keep an eye on the merge queue and dependabot PRs it should be easy enough to revert anyhow

Copy link
Contributor

openshift-ci bot commented Oct 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KPostOffice

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 1f9e9bf into project-codeflare:main Oct 23, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants