Skip to content

Conversation

deronnax
Copy link
Collaborator

get_timezone is not needed: settings.TIME_ZONE can be used and is guaranteed to exist and be valid. The fallback of APP_TIME_ZONE, despite it should never happen since TIME_ZONE necessarily exist, is misleading: nowhere in Django the default timezone is America/New York (it was the default parameter for the project generation years ago and it's not the case anymore, see the doc https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-TIME_ZONE).

Still a draft currently. Don't mind the "parallelize test runs" commit.

@github-actions github-actions bot added documentation Improvements or additions to documentation github-actions tests labels Mar 24, 2024
@adamspd
Copy link
Owner

adamspd commented Mar 25, 2024

The time zone function was a dirty fallback to set it to "America/New York" by default, since that's the one I needed. I'm pretty sure I put a TODO somewhere in the code to implement this the right way and remove the dirty code, but I didn't remember to do it.

I need to find a way to allow others to merge if the tests passed (I can't see if they were executed). Let me know when I can merge (once it's no longer a draft).

@deronnax
Copy link
Collaborator Author

deronnax commented Mar 25, 2024

I can't see if they were executed

Normally you should, once the workflow run has been granted. Btw, tests seem to not run on this MR, any idea why?

@adamspd
Copy link
Owner

adamspd commented Mar 25, 2024

I've been reading GitHub's documentation here and there, can't seem to find the reason why the tests are not running. I thought by adding the pull request target condition in the workflow as stated in the documentation and changing the configuration to allow them, it would work but apparently not :

name: Tests

on:
  push:
    branches-ignore:
      - 'main'
  workflow_dispatch:
  pull_request_target:
    types: [assigned, opened, synchronize, reopened]
    branches-ignore:
      - 'main'

@adamspd
Copy link
Owner

adamspd commented Mar 25, 2024

Still a draft currently

Still a draft ? Or can I proceed to merge it ?

It's not needed since settings.TIME_ZONE can be used and
is guaranteed to exist and be valid.
@deronnax
Copy link
Collaborator Author

I want to review and double-check some things, I'll tell you soon, probably tonight.

@adamspd
Copy link
Owner

adamspd commented Mar 27, 2024

Awesome.

@deronnax deronnax marked this pull request as ready for review March 27, 2024 11:31
@deronnax
Copy link
Collaborator Author

And the test job keeps not running and that's really a pain, I can't work efficiently without tests runs on my PR. It seems like we are hitting a not-so-uncommon corner case of Github, there is Github discussion on the subjects here. It seems to be related to CI job name and branch protection rules of the Github repo (which I can't see), would you please give one more try? Sorry and thank you.

@adamspd
Copy link
Owner

adamspd commented Mar 27, 2024

Yes, I'll check it out. I agree it's annoying. I'm wondering if adding you as collaborator (if you want it too) would solve the problem or if more permission would be required.

@adamspd
Copy link
Owner

adamspd commented Mar 27, 2024

@deronnax I tried updating the workflow name from "tests" to "testing" and change the name in the branch rule as explained in the link in your comment and updated the settings so anyone can see the worklow and have write and read permission, but it still shows here as "Waiting for status to be reported". I tried creating a new account and added it as a collaborator, and it has the right permissions needed to see the worklow, launch them and it has the push rights directly into the repository. So, if you're interested, I can add you, but I'm gonna need the email address you used in your github account. Once added you can remove the comment if you don't want your email address to show here.

In the meantime, I saw the tests ran well in the forked repo, I can merge it and then see afterwards.

@deronnax
Copy link
Collaborator Author

deronnax commented Mar 27, 2024

I think you can invite me with just my handle, @deronnax . OK for being part of the repo, until we figure out this problem at least.
Yes indeed tests ran well on the fork so let's merge (btw, how did you know for the fork?), but since I had some test failures locally, I wanted to be extra careful.

@adamspd
Copy link
Owner

adamspd commented Mar 27, 2024

I sent the invitation.

btw, how did you know for the fork?

2 reasons:

  1. GitHub has stats for the repo where you can see how many fork a repo has
  2. I participated in other repositories (golang mostly) and I had to fork to be able to send merge request.

@adamspd adamspd enabled auto-merge March 27, 2024 17:31
@adamspd adamspd disabled auto-merge March 27, 2024 17:32
@adamspd adamspd merged commit 175888d into adamspd:main Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation github-actions tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants