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

import https://github.com/adobe/rules_gitops/commit/0483dedecef451c68… #12

Merged
merged 1 commit into from Dec 5, 2023

Conversation

apesternikov
Copy link
Contributor

No description provided.

body = branch
}

if err := gitServer.CreatePR(branch, *prInto, title, body); err != nil {
log.Fatal("unable to create PR: ", err)

Choose a reason for hiding this comment

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

The error handling here is quite severe. Using log.Fatal will cause the program to immediately exit, which might not be the desired behavior in all cases. If the creation of a single PR fails, it might be better to log the error and continue with the next one, rather than stopping the entire process. Consider changing this to a less severe error handling method, such as log.Error, and continue with the next iteration. This way, a single failure won't stop the entire process.

Comment on lines +255 to +263
title := *prTitle
if title == "" {
title = fmt.Sprintf("GitOps deployment %s", branch)
}

err := gitServer.CreatePR(branch, *prInto, *prTitle, *prBody)
if err != nil {
body := *prBody
if body == "" {
body = branch
}

Choose a reason for hiding this comment

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

The default values for title and body are not very descriptive. If prTitle and prBody are not provided, they are set to "GitOps deployment {branch}" and "{branch}" respectively. This might not provide enough context for someone looking at the PR. Consider providing more descriptive default values or enforcing that these values are provided by the user.

@apesternikov apesternikov merged commit ec940f4 into main Dec 5, 2023
2 checks passed
@apesternikov apesternikov deleted the pr_naming_bug branch December 5, 2023 00:58
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.

None yet

2 participants