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.

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

Fix: Fix setting last updated commit when pushing and update when only auth repo updated #577

Merged
merged 3 commits into from
Jan 7, 2025

Conversation

renatav
Copy link
Collaborator

@renatav renatav commented Jan 4, 2025

Description (e.g. "Related to ...", etc.)

This PR contains 4 simple fixes:

  • After running a command which pushes to the auth repo, we update the last validated commit. However, since we only validated commits that are on the default branch, last validated commit should only be updated when pushing to this branch
  • Similarly, when traversing a repo's commit history, only traverse the default branch's history, even if a different branch is currently checked out.
  • Replaced an outdated check if a repo is a test repo
  • Determining from which commit the update of target repos should start was incorrect in cases when all target repositories were updated up to the same auth commit, but the auth repo was updated in the meantime. When some target repositories are excluded from the update, their last validated commits will be different and that case is not affected by this bug.

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Changelog has been updated, as needed (see CHANGELOG.md)

@renatav renatav changed the title Fix: Fix setting last updated commit when pushing and update update when only auth repo updated Fix: Fix setting last updated commit when pushing and update when only auth repo updated Jan 4, 2025
…y auth repo updated

If only the auth repo was updated, commits from which the update of target repos should start were not being determined correctly. Additionally, checking if repo is a test repo did not work for bare repos. Only update last validated commit if pushing to the default branch.
@renatav renatav force-pushed the renatav/fix-update-and-lvc branch from 08d74a2 to 12e9941 Compare January 4, 2025 04:55
@renatav renatav self-assigned this Jan 6, 2025
@renatav renatav requested a review from dgreisen January 7, 2025 01:40
if new_commit_branch:
new_commit = self.top_commit_of_branch(new_commit_branch)
if new_commit:
new_commit = self.commit(commit_msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: I love how much easier the type hints make it to go from zero to understanding the code.

suggestion (nonblocking): would it be insane to add a committish type or a hash type so that the type hints are more useful? right now a lot of strings are passed around, so have to look at code to see what that string is. I'm thinking a very simple class Committish(str): pass that would make it a lot clearer what was being returned/consumed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That could be useful, but I think it would be better to address that in a separate PR, as we'd want to update all occurrences of commits. I'll create an issue

@renatav renatav merged commit 4b46e8a into master Jan 7, 2025
8 checks passed
@renatav renatav deleted the renatav/fix-update-and-lvc branch January 7, 2025 17:50
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.

2 participants