Facade timezone fix redux #3252
Conversation
sgoggins
left a comment
sgoggins
left a comment
There was a problem hiding this comment.
Thorough testing demonstrates this improves collection speed, and thus far in three weeks of testing has reduced errors to zero.
There was a problem hiding this comment.
This PR seems to include changes that do not only flow one way (i.e. merges into feature branches, rather than feature branches merging into main).
Can the changes unique to this PR be rebased onto current main before merging?
it may also be possible to automate this via branch protection rules so that only main and release can be targets for PRs
Signed-off-by: Ulincsys <ulincsys@gmail.com>
The keys on this dictionary are defined in: analyzecommit.generate_commit_record() - Update reference to use proper 'cmt_author_timestamp' key - Add warning log when replacing TZdata to show commit hash Signed-off-by: Ulincsys <ulincsys@gmail.com>
Ulincsys
force-pushed
the
facade-timezone-fix-redux
branch
from
0ce574b to
aeaf09f
Compare
|
@MoralCode I've rebased the branch and force pushed. Personally speaking, I don't think it's always necessary to rebase. My understanding of the new development strategy was that changes should only ever flow into release, not that there were any restrictions on bidirectional merging with main. |
Yeah that might at least partly just be me making assumptions in the interest of just having one simple rule to follow rather than having to think about what the exceptions are. That said, I agree we don't always have to rebase. If this branch was based several commits back and only had your two commits and no merge conflicts, I'd be okay with just merging (into main, once approved) without a rebase. I think the part that we should avoid is adding back-merge commits into feature branches for the purpose of updating this feature branch when that update isn't needed for the functionality of this PR/to resolve a conflict. Bringing stuff from main like that makes the diffs bigger and requires differentiating during code review between which changes are new and need approval vs what has already been approved and is brought in via back-merge from main. |
MoralCode
left a comment
MoralCode
left a comment
There was a problem hiding this comment.
Currently working on testing this locally to see if it will fix any of the facade issues I see - might take a while though due to travel so if everyone else is cool with merging, then go for it
|
Sounds good. I think we can wait, let us know when you've had a chance to get to it. On our end, we saw that this seemingly also fixed the issue with facade getting stuck. I'm not entirely sure how this fixed that, but we were seeing it before, and we're no longer seeing it. I'm curious to see if you find the same thing on your end 👀 |
|
@MoralCode : I'm confused about the rebase discussion. This is a change to one file from a feature branch going into main. |
thats because it was force-pushed from 0ce574b to aeaf09f. if you click on the link for the original commit (the first one i listed), and then go to the parent commit, there's a merge commit that brings in changes from main that weren't needed for this PR to function - thats what was removed by the force push - turning this PR into a clean two commits on top of main that are easier to review |
Could this already be done? Or maybe I'm not understanding. I don't see a merge commit, but maybe I'm not seeing all the right flags. @MoralCode |
|
@sgoggins I removed the merge commits last week, so they are no longer present in this PR. @MoralCode Just a note: The merge commits in this branch did not originate from a PR on the GitHub side, they were produced from running Also, have you had a chance to test out the changes? If not, I think we may wish to move forward with merging anyways, as our testing has indicated no issues so far. |
despite loading in (on my local instance) a sample list of repos that are known to be problematic due to their size, I'm not seeing any failures (just the large jobs getting stuck still) - sounds like this PR is tackling those kinds of outright failures, so it seems good to me! its probably not the most thorough testing in the world but I don't see a good reason to keep holding up this PR. Feel free to ship it! |
3 participants
Description
Signed commits