Skip to content

Conversation

@ziadhany
Copy link
Collaborator

@ziadhany ziadhany commented Oct 6, 2025

  • Add a default print logger to prevent exceptions
  • Bump minecode_pipelines version to 0.0.1b24

Add a default print logger to prevent exceptions
Bump minecode_pipelines version to 0.0.1b24

Signed-off-by: ziad hany <[email protected]>
Copy link
Member

@keshav-space keshav-space left a comment

Choose a reason for hiding this comment

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

@ziadhany See comment below.
Also why use print? why not simply pass self.log while calling commit_and_push_changes? Spraying custom logging mechanisms in pipeline functions defeats the whole purpose of having a centrally controlled logger managed by the pipeline, which helps in proper dissemination of pipeline logs to the run instance and UI.

Comment on lines +211 to +224
if not commit_message:
author_name = settings.FEDERATEDCODE_GIT_SERVICE_NAME
author_email = settings.FEDERATEDCODE_GIT_SERVICE_EMAIL

purls = "\n".join(purls)
commit_message = textwrap.dedent(f"""\
Add {mine_type} results for:
{purls}
Tool: {tool_name}@v{tool_version}
Reference: https://{settings.ALLOWED_HOSTS[0]}
Signed-off-by: {author_name} <{author_email}>
""")
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why we need to compute commit_message here? when we already take care of missing commit message in commit_changes function.

@ziadhany
Copy link
Collaborator Author

ziadhany commented Oct 6, 2025

@keshav-space The main error comes from If a pipeline fails before updating a checkpoint, when a new pipeline runs, it starts and tries to commit some data that was already pushed. This raises the error:
Nothing to commit, working tree clean This disrupts the pipeline and causes it to fail

One way to handle this is to use commit_and_push_changes, since it pushes and commits at the same time and can catch GitCommandError.

Why add a print logger by default?

  • This prevents pipeline disruption and avoids unreadable errors such as 'NoneType' object is not callable.' Since the logger is optional, using a print statement with a False return provides a simple and safe fallback.

Why add commit_and_push_changes?

  • To avoid duplicating commit messages for every ecosystem (e.g., CRAN, Swift, Composer).

@keshav-space
Copy link
Member

@ziadhany

The main error comes from If a pipeline fails before updating a checkpoint, when a new pipeline runs, it starts and tries to commit some data that was already pushed. This raises the error: Nothing to commit, working tree clean This disrupts the pipeline and causes it to fail

One way to handle this is to use commit_and_push_changes, since it pushes and commits at the same time and can catch GitCommandError.

Please use commit_and_push_changes we already handle empty commit scenario see #1888, no special error handling is needed on call side.

Why add a print logger by default?

This prevents pipeline disruption and avoids unreadable errors such as 'NoneType' object is not callable.' Since the logger is optional, using a print statement with a False return provides a simple and safe fallback.

Let's not use print as default logger and if needed we should instead make logger required field that would be much simpler.

Why add commit_and_push_changes?

To avoid duplicating commit messages for every ecosystem (e.g., CRAN, Swift, Composer).

I don't get you here.
Can you explain why the below fallback commit is not sufficient for CRAN, Swift and Composer?

if not commit_message:
author_name = settings.FEDERATEDCODE_GIT_SERVICE_NAME
author_email = settings.FEDERATEDCODE_GIT_SERVICE_EMAIL
purls = "\n".join(purls)
commit_message = f"""\
Add {mine_type} results for:
{purls}
Tool: {tool_name}@v{tool_version}
Reference: https://{settings.ALLOWED_HOSTS[0]}
Signed-off-by: {author_name} <{author_email}>
"""

And why would we need to compute default commit message in commit_and_push_changes when we already have default commit message in downstream commit_changes function?

@ziadhany
Copy link
Collaborator Author

ziadhany commented Oct 6, 2025

@keshav-space Ok, I get your point. I didn’t notice we already have a default commit message, and I will close this PR.

@ziadhany ziadhany closed this Oct 6, 2025
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.

3 participants