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 .Phony to Makefile #77

Merged
merged 2 commits into from
May 23, 2023
Merged

Conversation

razo7
Copy link
Member

@razo7 razo7 commented May 22, 2023

Similar to NMO' PR , NHC's PR and SNR's PR .

  • Add .PHONY to each target.
  • Reorganize targets based on categorizes.

@openshift-ci openshift-ci bot requested review from clobrano and mshitrit May 22, 2023 09:00
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: razo7

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

@razo7 razo7 changed the title Phony makefile Add .Phony to Makefile May 22, 2023
To avoid a conflict with a file of the same name, and to improve performance.
@mshitrit
Copy link
Member

/lgtm
/hold
giving other chance to review

@mshitrit
Copy link
Member

Any particular reason for moving the locations of opm and mutation-ci blocks ?

@razo7
Copy link
Member Author

razo7 commented May 23, 2023

Any particular reason for moving the locations of opm and mutation-ci blocks ?

I have reorganized these targets based on categorizes.

  • operator-sdk and opm are targets which are more suitable IMHO to be part of Build Dependencies rather than Working with Bundle (which is a new category) category.
  • Same goes to test-mutation-ci to be part of Targets used by CI category.

Feel free to share your opinion, since this is only my preference.

Clearer grouping of targets
@clobrano
Copy link
Contributor

Damn, I forgot to push the review yesterday

image

basically the same questions as Michael, plus I thought the mutation tests were destined to be removed, aren't they?

Not strong opinion, but I see operator-sdk and opm more like build dependencies, like they were before this change.

@razo7
Copy link
Member Author

razo7 commented May 23, 2023

basically the same questions as Michael, plus I thought the mutation tests were destined to be removed, aren't they?

I do remeber that there was a talk about removing it, but ATM MDR still use it. Therefore, I didn't intend to remove target, just to rearrange or group the targets a little bit different.
If you want to remove it, then I would suggest, for better separation, to do it in a different PR.

Not strong opinion, but I see operator-sdk and opm more like build dependencies, like they were before this change.

They remain build dependencies after this PR. I have moved them and created a new category for Bundle (Working with Bundle), since they were other targets under build dependencies which aren't related to build dependencies, and more suitable IMHO to something like Working with Bundle.

@clobrano
Copy link
Contributor

but ATM MDR still use it.

my bad, I didn't notice it was a move, not add

They remain build dependencies after this PR. I have moved them and created a new category for Bundle (Working with Bundle), since they were other targets under build dependencies which aren't related to build dependencies, and more suitable IMHO to something like Working with Bundle.

that's ok for me
/lgtm
adding hold, just to give the others time for a feedback. Feel free to unhold
/hold

@razo7
Copy link
Member Author

razo7 commented May 23, 2023

/unhold

@openshift-merge-robot openshift-merge-robot merged commit 562c6b8 into medik8s:main May 23, 2023
2 checks 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.

None yet

4 participants