Skip to content

fix: refresh_mvCountIssueRatingsServiceId_proc#1187

Merged
MR2011 merged 2 commits into
mainfrom
kanstantsinbuklis-sap/issue-1177/fix-refresh-proc
May 5, 2026
Merged

fix: refresh_mvCountIssueRatingsServiceId_proc#1187
MR2011 merged 2 commits into
mainfrom
kanstantsinbuklis-sap/issue-1177/fix-refresh-proc

Conversation

@kanstantsinbuklis-sap
Copy link
Copy Markdown
Collaborator

Description

In this PR I've changed the kind of join from left to right because using left join in the result select we can get NULL

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Related Tickets & Documents

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • Separate ticket for tests # (issue/pr)

Added to documentation?

  • 📜 README.md
  • 🤝 Documentation pages updated
  • 🙅 no documentation needed
  • (if applicable) generated OpenAPI docs for CRD changes

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Collaborator

@michalkrzyz michalkrzyz left a comment

Choose a reason for hiding this comment

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

Can we split this change to have clear view what was fixed? autocode change hides productivity...and it does not look better at all. I would not merge such changes.

Copy link
Copy Markdown
Collaborator

@michalkrzyz michalkrzyz left a comment

Choose a reason for hiding this comment

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

We should split this change. Please avoid combining fixes or redesigns with large-scale refactoring or auto-formatting.

Let's introduce a rule: no deep refactoring or formatting-only changes in the same PR as functional fixes.

When we fix or redesign something, there’s a real chance we may need to revert it. The more files and lines touched (especially by formatting changes), the harder and riskier that rollback becomes. It also makes code review and debugging significantly more difficult.

Please separate the functional changes from the formatting changes into distinct PRs.

@MR2011
Copy link
Copy Markdown
Collaborator

MR2011 commented May 5, 2026

We should split this change. Please avoid combining fixes or redesigns with large-scale refactoring or auto-formatting.

Let's introduce a rule: no deep refactoring or formatting-only changes in the same PR as functional fixes.

When we fix or redesign something, there’s a real chance we may need to revert it. The more files and lines touched (especially by formatting changes), the harder and riskier that rollback becomes. It also makes code review and debugging significantly more difficult.

Please separate the functional changes from the formatting changes into distinct PRs.

I agree. Let's separate these changes please.

@kanstantsinbuklis-sap kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1177/fix-refresh-proc branch from 3224547 to 96cadfa Compare May 5, 2026 09:21
@kanstantsinbuklis-sap
Copy link
Copy Markdown
Collaborator Author

kanstantsinbuklis-sap commented May 5, 2026

Removed formatting that was made by pre-commit hook
cc @MR2011 @michalkrzyz

}
}

func (s *SqlDatabase) runPostMigrationCleanupRoutineInBackground(procs []string) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this change most probably will corrupt some functionality. We need to keep this goroutine in background otherwise app will not start up until post migration is not done.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, checked on the production dump and it takes lots of time, will return this goroutine

Signed-off-by: Kanstantsin Buklis <[email protected]>
@kanstantsinbuklis-sap kanstantsinbuklis-sap force-pushed the kanstantsinbuklis-sap/issue-1177/fix-refresh-proc branch from 5dceb11 to 57764aa Compare May 5, 2026 10:23
@kanstantsinbuklis-sap kanstantsinbuklis-sap linked an issue May 5, 2026 that may be closed by this pull request
@MR2011 MR2011 merged commit 2d9499d into main May 5, 2026
1 check failed
@MR2011 MR2011 deleted the kanstantsinbuklis-sap/issue-1177/fix-refresh-proc branch May 5, 2026 13:08
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.

fix: refresh_mvCountIssueRatingsServiceId_proc

4 participants