Skip to content

Made changes to fix the head sha commit in diff comment#56

Merged
banginji merged 2 commits intomainfrom
fix-diff-msg-commit-sha
Mar 13, 2026
Merged

Made changes to fix the head sha commit in diff comment#56
banginji merged 2 commits intomainfrom
fix-diff-msg-commit-sha

Conversation

@banginji
Copy link
Collaborator

1- Made changes to GithubActionsSettings to now lookup the pr number
2- Renamed head commit sha var names to align
3- Modified the diff comment to indicate head sha commit

This fixes #55

1- Made changes to GithubActionsSettings to now lookup the pr number
2- Renamed head commit sha var names to align
3- Modified the diff comment to indicate head sha commit

Signed-off-by: banginji <7316646+banginji@users.noreply.github.com>
@banginji banginji requested a review from colindean March 11, 2026 12:01
Comment on lines +36 to +37
def set_pr_lookup_service(self, service: PrLookupService) -> None:
object.__setattr__(self, "_pr_lookup_service", service)
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Why can't this be self._pr_lookup_service = service?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to how it was before; had to define it as private attribute in vela and github actions settings

Comment on lines +47 to +48
logger.warning("PR lookup requested before service configured; returning None")
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue (non-blocking): Can the rest of the process continue if the pr_num isn't available? If it can, this is fine. If it can't, then this should raise an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Wherever pr_num is called, we've a check to see whether it is None because it's not a blocker

logger.debug("VelaSettings event_name={}", self.event_name)

def set_pr_lookup_service(self, service: PrLookupService) -> None:
self._pr_lookup_service = service
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: ☝️ like this was

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to set it like how it did before

sigil_envvar: ClassVar[str]
"""The envvar in this will always be present when this settings is valid."""

_pr_lookup_service: PrLookupService | None = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: Ah, I see, this was promoted to the base class from VelaSettings…

1- Updated the handling of _pr_lookup_service; defined it and _pr_num_cached as private attributes in both vela and github actions settings

Signed-off-by: banginji <7316646+banginji@users.noreply.github.com>
@banginji banginji merged commit 251a4cb into main Mar 13, 2026
9 checks passed
@banginji banginji deleted the fix-diff-msg-commit-sha branch March 13, 2026 00:09
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.

Commit hash not resolving for the head sha in diff comment

2 participants