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

Fixes #RHIROS-1402 - Fixed payload creation for kruize updateResult API #142

Merged
merged 2 commits into from
Oct 27, 2023

Conversation

patilsuraj767
Copy link
Collaborator

No description provided.

@patilsuraj767
Copy link
Collaborator Author

/retest

@saltgen saltgen self-requested a review October 26, 2023 11:57
@patilsuraj767
Copy link
Collaborator Author

/retest

@@ -238,3 +242,19 @@ func ProcessReport(msg *kafka.Message) {
}
}
}

func sliceK8sObjectToChunks(k8s_objects []kruizePayload.UpdateResult) [][]kruizePayload.UpdateResult {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func sliceK8sObjectToChunks(k8s_objects []kruizePayload.UpdateResult) [][]kruizePayload.UpdateResult {
func sliceUpdatePayloadToChunks(k8s_objects []kruizePayload.UpdateResult) [][]kruizePayload.UpdateResult {

Copy link
Contributor

Choose a reason for hiding this comment

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

@patilsuraj767 Additionally, I didn't get the reason for moving this function to current file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was getting linting issue because of cyclic imports. So I moved the function out of utils file.

Copy link
Contributor

@saltgen saltgen Oct 27, 2023

Choose a reason for hiding this comment

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

I think we should try debugging the cyclic import error, in the interest of code maintenance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Method signature is changed now we are using kruizePayload.UpdateResult type in the method. So it was not possible to import that types in utils package because utils package(some functions from utils) is already imported in types package.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use Dependency Inversion Principle to resolve this issue, for now I'm unblocking this PR.

Copy link
Collaborator Author

@patilsuraj767 patilsuraj767 Oct 27, 2023

Choose a reason for hiding this comment

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

I had a look into Dependency Inversion Principle not sure how it fits in for this case but thanks for unblocking the PR. I will request you if possible you can implement it and create the new PR, will be happy to learn new things.
I referred - https://hackernoon.com/go-design-patterns-an-introduction-to-solid

@patilsuraj767
Copy link
Collaborator Author

/retest

@patilsuraj767
Copy link
Collaborator Author

PR checks are failing because of some unknown issue. This has been reported to the QE team.
I have tested the PR in ephemeral environment manually and everything look good. Hence merging the PR.

@patilsuraj767 patilsuraj767 merged commit 4b03562 into RedHatInsights:main Oct 27, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants