Skip to content

Use sed command to replace MCAD repo reference in Makefile #276

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

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

sutaakar
Copy link
Contributor

@sutaakar sutaakar commented Sep 6, 2023

Issue link

What changes have been made

Kustomize fn run is using Docker. This approach is incompatible with OpenShift CI, which doesn't provide Docker environment.
To make operator deployment being OpenShift CI friendly (and to remove a need to have specifically Docker installed for various Makefile actions) replacing the function with sed command providing same functionality.

Verification steps

Execute for example make install without Docker.

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

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

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

I would expect changes in the config/crd/mcad/kustomization.yaml, are these missing?

@sutaakar
Copy link
Contributor Author

sutaakar commented Sep 6, 2023

@astefanutti
config/crd/mcad/kustomization.yaml contains placeholder, I have left there kustomize metadata as it doesn't block or affect anything
The value itself is replaced just when needed (deployment using Makefile or bundle build).

@astefanutti
Copy link
Contributor

@sutaakar oh I see, my sed-fu is not as strong as yours :).

So it won't work on macOS out of the box. If we don't want to adjust the command depending on the OS, we can mandate gnu sed to be installed and used on macOS.

@sutaakar
Copy link
Contributor Author

sutaakar commented Sep 6, 2023

That would be the best :)

@astefanutti
Copy link
Contributor

That would be the best :)

Maybe a quick note in the README can be added, WDYT?

@sutaakar
Copy link
Contributor Author

sutaakar commented Sep 6, 2023

@astefanutti added a note in readme

@astefanutti
Copy link
Contributor

/lgtm

@astefanutti
Copy link
Contributor

/approve

@openshift-ci
Copy link

openshift-ci bot commented Sep 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astefanutti

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

@openshift-ci openshift-ci bot added the approved label Sep 6, 2023
@openshift-merge-robot openshift-merge-robot merged commit c093d58 into project-codeflare:main Sep 6, 2023
@sutaakar sutaakar deleted the sed branch September 7, 2023 07:06
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.

3 participants