Skip to content

Conversation

joshuadavidthomas
Copy link
Contributor

@joshuadavidthomas joshuadavidthomas commented Oct 27, 2022

Description of the Change

Adds an additional job matrix that includes all the Django versions this package supports and excludes certain combinations of Python and Django from running. Also removes the max-parallel restriction for parallel CI jobs.

This should dramatically speed up CI tests, cutting almost 20 minutes from the total runtime.

Note: I have another open PR (#1218) that adds Python 3.11 to CI and tox. Either this PR or that one will need to be rebased once either one is accepted and merged.

Before

~25 minutes

image

After

~8 minutes

image

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #1219 (d8318db) into master (13538a6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1219   +/-   ##
=======================================
  Coverage   96.97%   96.97%           
=======================================
  Files          31       31           
  Lines        1821     1821           
=======================================
  Hits         1766     1766           
  Misses         55       55           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Andrew-Chen-Wang
Copy link
Contributor

pre-commit needs fixing

@n2ygk
Copy link
Contributor

n2ygk commented Oct 31, 2022

@joshuadavidthomas please rebase now that #1218 is merged.

@n2ygk
Copy link
Contributor

n2ygk commented Oct 31, 2022

@joshuadavidthomas I see some required builds that are expected but waiting to status. Is this an artifact of the changes and safe to merge anyway?

image

Copy link
Contributor

@n2ygk n2ygk 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 but. think we'll have to merge and deploy to confirm.

@joshuadavidthomas
Copy link
Contributor Author

@joshuadavidthomas I see some required builds that are expected but waiting to status. Is this an artifact of the changes and safe to merge anyway?

image

Yes, I believe so.

@joshuadavidthomas
Copy link
Contributor Author

joshuadavidthomas commented Oct 31, 2022

I think the branch protections are getting in the way, I may need to adjust the workflow to have a 'sum up' job at the end to check if any jobs failed and then the branch protection can look for that instead of the individual build jobs.

community/community#4324

@dopry
Copy link
Member

dopry commented Dec 5, 2022

@n2ygk @Andrew-Chen-Wang who has access to the branch protection rules? I suspect the reason the Expect 3.10, 3.7, 3.8, and 3.9 are hanging up the build process is that those tests are explicitly listed as required. We'll need to update that list of tests when merging this.

@Andrew-Chen-Wang
Copy link
Contributor

@jezdez has settings access. Not sure whether @n2ygk does.

Copy link
Contributor

@n2ygk n2ygk left a comment

Choose a reason for hiding this comment

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

Thanks!

@dopry
Copy link
Member

dopry commented Dec 7, 2022

@joshuadavidthomas do you have the inclination to add a success job to the workflow as indicated in jazzband/help#321 (comment) by hugovk?

 success:
    needs: build
    runs-on: ubuntu-latest
    name: Test successful
    steps:
      - name: Success
        run: echo Test successful

@joshuadavidthomas
Copy link
Contributor Author

@joshuadavidthomas do you have the inclination to add a success job to the workflow as indicated in jazzband/help#321 (comment) by hugovk?

 success:
    needs: build
    runs-on: ubuntu-latest
    name: Test successful
    steps:
      - name: Success
        run: echo Test successful

Yeah, absolutely. Who should I ping when I’m done?

@dopry
Copy link
Member

dopry commented Dec 8, 2022

@joshuadavidthomas , I think only @jezdez has the necessary permissions. maybe @hugovk does as well. I'll be happy to review the code though.

@hugovk
Copy link

hugovk commented Dec 8, 2022

Only roadies have access and @jezdez is the only roadie, see jazzband/help#196. Hopefully he can get to this soon 👍

@jezdez
Copy link
Contributor

jezdez commented Dec 12, 2022

I can't add the summary job before it's run at least once on GitHub here. Let me know when you've made the change. I dislike removing branch protection details for security critical projects like this.

@n2ygk
Copy link
Contributor

n2ygk commented Feb 13, 2023

@jezdez I think you need to override the protections to merge this given these new build steps statuses are not reporting even though they show as successful in Actions:

image

I'm not knowledgeable about GH Actions so defer to you, @joshuadavidthomas and @dopry

@dopry
Copy link
Member

dopry commented Feb 13, 2023

@jezdez I've added the success job. It has ran, https://github.com/jazzband/django-oauth-toolkit/actions/runs/4165563688/jobs/7209027629. Can you update the checks on the branch protection to reference it now instead of the individual builds?

@n2ygk n2ygk merged commit bd865d6 into django-oauth:master Feb 16, 2023
@n2ygk
Copy link
Contributor

n2ygk commented Feb 16, 2023

Thanks @jezdez!!!

@joshuadavidthomas
Copy link
Contributor Author

🎉

Sorry I dropped the ball on updating the PR, work's been crazy lately. Thanks to @dopry and @jezdez for pushing it over the finish line!

@dopry
Copy link
Member

dopry commented Feb 17, 2023

@joshuadavidthomas no worries. I know how it is. :) Thanks for getting this done in the first place. It's really appreciated.

@n2ygk
Copy link
Contributor

n2ygk commented Dec 19, 2023

#1376

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants