Skip to content

Conversation

@VanillaSpoon
Copy link
Contributor

@VanillaSpoon VanillaSpoon commented Aug 3, 2023

Issue link

closes #115

What changes have been made

This pr includes a refactor of machinepools.go separating the logic to create an ocm connection. Improving readability :)

The pr also include minor refactors of appwrapper_controller.go reducing nested if/else statements.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • Testing is not required for this change

Copy link
Contributor

@anishasthana anishasthana left a comment

Choose a reason for hiding this comment

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

/lgtm

Is there an image that you've deployed somewhere that I can see in action?

@VanillaSpoon
Copy link
Contributor Author

Hey Anish,
Thanks for the review, feel free to use this image: quay.io/rh_ee_egallina/instascale:OCMConnection

@VanillaSpoon VanillaSpoon force-pushed the refactorOCMConnection branch from ad4ab77 to 5c769fc Compare August 14, 2023 10:28
@openshift-ci openshift-ci bot removed the lgtm label Aug 14, 2023
@asm582
Copy link
Member

asm582 commented Aug 14, 2023

@VanillaSpoon Thanks for the PR, did you manually test these changes?

@VanillaSpoon
Copy link
Contributor Author

Hey @asm582 ,
Yes, I have tested the changes manually on my side :)

@asm582
Copy link
Member

asm582 commented Aug 14, 2023

Thanks @VanillaSpoon If you are doing manual tests, request that we add logs or log snippet to the PR going forward

@asm582
Copy link
Member

asm582 commented Aug 14, 2023

/approve

@openshift-ci
Copy link

openshift-ci bot commented Aug 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asm582

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link
Contributor

@anishasthana anishasthana left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 23, 2023
@openshift-merge-robot openshift-merge-robot merged commit ea5942d into project-codeflare:main Aug 23, 2023
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.

Refactor machinepools.go to avoid ocm connection duplication

4 participants