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

Add bundle-run Makefile target #28

Merged

Conversation

clobrano
Copy link
Contributor

Add Makefile's target to run bundle via operator-sdk.

Signed-off-by: Carlo Lobrano [email protected]

@openshift-ci openshift-ci bot requested review from mshitrit and razo7 March 23, 2023 09:27
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 23, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano

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

@mshitrit
Copy link
Member

Do we plan to use this for anything other than to deploy operators on our dev clusters ?

@clobrano
Copy link
Contributor Author

Do we plan to use this for anything other than to deploy operators on our dev clusters ?

And maybe to simplify testing for others

@clobrano
Copy link
Contributor Author

/test openshift-e2e

@mshitrit
Copy link
Member

maybe to simplify testing for others

I am not sure that code that is intended purely for dev purposes belongs here, my first intuition is that this would belong in a more internal space.

@razo7
Copy link
Member

razo7 commented Mar 27, 2023

/retest

@clobrano
Copy link
Contributor Author

I am not sure that code that is intended purely for dev purposes belongs here, my first intuition is that this would belong in a more internal space.

That's a good point and I don't have strong opinion about it

@mshitrit
Copy link
Member

I am not sure that code that is intended purely for dev purposes belongs here, my first intuition is that this would belong in a more internal space.

That's a good point and I don't have strong opinion about it

@slintes , @razo7 WDYT, do you have any preference ?

@slintes
Copy link
Member

slintes commented Mar 28, 2023

What's the difference to all the other targets, isn't the complete Makefile for devs? 🤔

@mshitrit
Copy link
Member

What's the difference to all the other targets, isn't the complete Makefile for devs? thinking

I guess that depends on how you define dev , from my perspective most of the targets have to do with testing/deploying the operator which are necessary to the CI.
IMO this different than a target which is used just to deploy on my local dev cluster.

Definitely not something I feel strongly about though, and IIUC other objections, so we can probably merge ahead.

Makefile Outdated
Comment on lines 321 to 323
.PHONY: bundle-run
bundle-run: operator-sdk
$(OPERATOR_SDK) -n openshift-operators run bundle $(BUNDLE_IMG)
Copy link
Member

@razo7 razo7 Mar 29, 2023

Choose a reason for hiding this comment

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

I am not sure that code that is intended purely for dev purposes belongs here, my first intuition is that this would belong in a more internal space.

That's a good point and I don't have strong opinion about it

@slintes , @razo7 WDYT, do you have any preference ?

I join some of us who I believe agree that this is kind of gray area, if this target is needed or in the right place. I don't mind having it.
Maybe just change the bundle name to include something about that specific namespace, -n openshift-operators or even remove it from the target and set is as a variable that we can export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A variable we can export seems a good improvement

@clobrano
Copy link
Contributor Author

I added this target to the "Bundle" section just because it refers to bundle, but would the "Development" section fit better?
https://github.com/medik8s/fence-agents-remediation/blob/main/Makefile#L138

@slintes
Copy link
Member

slintes commented Mar 29, 2023

What's the difference to all the other targets, isn't the complete Makefile for devs? thinking

I guess that depends on how you define dev , from my perspective most of the targets have to do with testing/deploying the operator which are necessary to the CI. IMO this different than a target which is used just to deploy on my local dev cluster.

ok, let me rephrase 😉
"IMHO the Makefile helps developers and CI to build and deploy the operator" 😎

A variable we can export seems a good improvement

👍🏼 (but openshift-operators is a good default value IMHO, as long as most people use it on OpenShift)

but would the "Development" section fit better?

I have no preferance 🤷🏼‍♂️

@razo7
Copy link
Member

razo7 commented Mar 29, 2023

I added this target to the "Bundle" section just because it refers to bundle, but would the "Development" section fit better?
https://github.com/medik8s/fence-agents-remediation/blob/main/Makefile#L138

I don't mind of using the current location

Add Makefile's target to run bundle via operator-sdk.

Signed-off-by: Carlo Lobrano <[email protected]>
@clobrano clobrano force-pushed the infra/make-run-bundle-target/0 branch from 42e6f4c to 4df8c92 Compare March 29, 2023 10:42
@razo7
Copy link
Member

razo7 commented Mar 29, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Mar 29, 2023
@openshift-merge-robot openshift-merge-robot merged commit dfe3a39 into medik8s:main Mar 29, 2023
@clobrano clobrano deleted the infra/make-run-bundle-target/0 branch March 29, 2023 12:24
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.

5 participants