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.

Sign up for GitHub

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

Jump to bottom

In TestViewRepo2, convert computed timezones to local time #24579

Merged
merged 4 commits into from
May 8, 2023
Merged

In TestViewRepo2, convert computed timezones to local time #24579

merged 4 commits into from
May 8, 2023

Conversation

kousu
Copy link
Contributor

@kousu kousu commented May 7, 2023 •
edited
Loading

This fixes up #10446; in that, the expected timezone was changed to the local timezone, but the computed timezone was left in UTC.

The result was this failure, when run on a non-UTC system:

	Diff:
    --- Expected
    +++ Actual
    @@ -5,3 +5,3 @@
       commitMsg: (string) (len=12) "init project",
    -  commitTime: (string) (len=29) "Wed, 14 Jun 2017 09:54:21 EDT"
    +  commitTime: (string) (len=29) "Wed, 14 Jun 2017 13:54:21 UTC"
      },
    @@ -11,3 +11,3 @@
       commitMsg: (string) (len=12) "init project",
    -  commitTime: (string) (len=29) "Wed, 14 Jun 2017 09:54:21 EDT"
    +  commitTime: (string) (len=29) "Wed, 14 Jun 2017 13:54:21 UTC"
      }
    Test:       	TestViewRepo2

I assume this was probably missed since the CI servers all run in UTC?

The Format() string "Mon, 02 Jan 2006 15:04:05 UTC" was incorrect: 'UTC' isn't recognized as a variable placeholder, but was just being copied verbatim. It should use 'MST' in order to command Format() to output the attached timezone, which is what time.RFC1123 has.

@kousu
In TestViewRepo2, convert timezones to local time
f7ad823 
This fixes up go-gitea#10446;
in that, the *expected* timezone was changed to the local timezone,
but the computed timezone was left in UTC.

The result was this failure, when run on a non-UTC system:

	Diff:
    --- Expected
    +++ Actual
    @@ -5,3 +5,3 @@
       commitMsg: (string) (len=12) "init project",
    -  commitTime: (string) (len=29) "Wed, 14 Jun 2017 09:54:21 EDT"
    +  commitTime: (string) (len=29) "Wed, 14 Jun 2017 13:54:21 UTC"
      },
    @@ -11,3 +11,3 @@
       commitMsg: (string) (len=12) "init project",
    -  commitTime: (string) (len=29) "Wed, 14 Jun 2017 09:54:21 EDT"
    +  commitTime: (string) (len=29) "Wed, 14 Jun 2017 13:54:21 UTC"
      }
    Test:       	TestViewRepo2

I assume this was probably missed for the last couple months
since the CI servers all run in UTC?

The Format() string "Mon, 02 Jan 2006 15:04:05 UTC" was incorrect: 'UTC' isn't
recognized as a variable placeholder, but was just being copied verbatim. It
should use 'MST' in order to command Format() to output the attached timezone,
which is what `time.RFC1123` has.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 7, 2023
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 7, 2023
@kousu kousu changed the title In TestViewRepo2, convert timezones to local time In TestViewRepo2, convert computed timezones to local time May 7, 2023
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 7, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 8, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 8, 2023
@lunny lunny enabled auto-merge (squash) May 8, 2023 06:40
@silverwind
Copy link
Member

silverwind commented May 8, 2023 •
edited
Loading

Wonder why @GiteaBot does not rebase here. Apparently I can't do it from GitHub UI either.

@yardenshoham
Copy link
Member

Maintainers don't have push access here

@lunny
Copy link
Member

lunny commented May 8, 2023 •
edited
Loading

@kousu please rebase the branch please update the branch. :)

@delvh delvh added the reviewed/prioritize-merge PR is in the merge queue. Merge as soon as possible, i.e. as edits by maintainers are not enabled label May 8, 2023
@yardenshoham yardenshoham added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label May 8, 2023
@wxiaoguang
Copy link
Contributor

Wonder why @GiteaBot does not rebase here. Apparently I can't do it from GitHub UI either.

The fork is from an org. GitHub doesn't allow maintainer to edit the org fork's PRs.

If I was the owner, I would merge this simple PR directly to save everyone's time. It doesn't make sense to wait again and again.

@lunny lunny disabled auto-merge May 8, 2023 13:07
@lunny lunny merged commit 3d266dd into go-gitea:main May 8, 2023
@GiteaBot GiteaBot added this to the 1.20.0 milestone May 8, 2023
@GiteaBot GiteaBot removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. reviewed/prioritize-merge PR is in the merge queue. Merge as soon as possible, i.e. as edits by maintainers are not enabled labels May 8, 2023
@kousu
Copy link
Contributor Author

kousu commented May 8, 2023

The fork is from an org. GitHub doesn't allow maintainer to edit the org fork's PRs.

Yep. I've run into this with every PR I've submitted to you so far. @jolheiser explained

Nope, not your fault, I believe it's a GitHub thing that doesn't let us update it because it's from an org vs a user account.

So thank you for just going ahead and squash-merging it in. Plus I'm in a different timezone than most of you, so I wouldn't have gotten to it for a while!

By the way,

@kousu please rebase the branch

Is this policy now? In this I was told off for using rebases:

why forcepush, that make it hard to review

@kousu kousu deleted the TestViewRepo2-timezone branch May 8, 2023 22:32
zjjhot added a commit to zjjhot/gitea that referenced this pull request May 9, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
2090e93 
* upstream/main:
  Add Gitea Profile Readmes (go-gitea#23260)
  Make diff view full width again (go-gitea#24598)
  Add permission check for moving issue action in project view page (go-gitea#24589)
  Update JS dependencies, add new eslint rules (go-gitea#24597)
  Filters for GetAllCommits (go-gitea#24568)
  [skip ci] Updated translations via Crowdin
  Attach a tooltip to the action control button (go-gitea#24595)
  Improve Gitea's web context, decouple "issue template" code into service package (go-gitea#24590)
  Support markdown editor for issue template (go-gitea#24400)
  Do not select line numbers when selecting text from the action run logs (go-gitea#24594)
  In TestViewRepo2, convert computed timezones to local time (go-gitea#24579)
  Fix close org projects (go-gitea#24588)
  Rewrite queue (go-gitea#24505)
  Split "modules/context.go" to separate files (go-gitea#24569)
  Pass 'not' to commit count (go-gitea#24473)
  Refresh the refernce of the closed PR when reopening (go-gitea#24231)
@yardenshoham
Copy link
Member

Don't rebase, lunny and silverwind meant merge

@silverwind
Copy link
Member

silverwind commented May 9, 2023 •
edited
Loading

Is this policy now? In #22177 (comment) I was told off for using rebases:

Merge, yes. Gitea repo has a branch protection rule that a PR can only be merged when it is strictly descending off the main branch tip (HEAD), so PRs generally need a few merges before they can land and if we can't push those merges, it can only be manually merged on CLI, I think that's what @lunny did here.

@lunny
Copy link
Member

lunny commented May 9, 2023

The fork is from an org. GitHub doesn't allow maintainer to edit the org fork's PRs.

Yep. I've run into this with every PR I've submitted to you so far. @jolheiser explained

Nope, not your fault, I believe it's a GitHub thing that doesn't let us update it because it's from an org vs a user account.

So thank you for just going ahead and squash-merging it in. Plus I'm in a different timezone than most of you, so I wouldn't have gotten to it for a while!

By the way,

@kousu please rebase the branch

Is this policy now? In this I was told off for using rebases:

why forcepush, that make it hard to review

I made a mistake and I have corrected it.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny approved these changes

@yardenshoham yardenshoham yardenshoham approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.
8 participants
@kousu @silverwind @yardenshoham @lunny @wxiaoguang @delvh @techknowlogick @GiteaBot