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

fix: default goal in makefile and add goal for unit tests #475

Merged
merged 2 commits into from
May 17, 2024

Conversation

xperimental
Copy link
Contributor

Three changes, but all in the Makefile:

  • Fix git command to ignore external diff tools
  • Fix definition of default target
  • Add a target for unit tests (optional, but I like being able to run tests with a simple command)

I can split this up into separate PRs, if you like.

@xperimental xperimental self-assigned this May 8, 2024
@xperimental xperimental requested a review from a team as a code owner May 8, 2024 16:08
@xperimental xperimental requested review from jan--f and sthaha and removed request for a team May 8, 2024 16:08
@openshift-ci openshift-ci bot requested a review from simonpasquier May 8, 2024 16:08
@xperimental xperimental force-pushed the fixes-makefile branch 2 times, most recently from 841e7e8 to b67b884 Compare May 8, 2024 16:15
@@ -15,10 +15,14 @@ OSD_E2E_TEST_HARNESS_IMG=$(IMAGE_BASE)-test-harness:$(VERSION)
OSD_E2E_TEST_HARNESS_IMG_LATEST=$(IMAGE_BASE)-test-harness:latest

# running `make` builds the operator (default target)
all: operator
.DEFAULT_GOAL := operator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Afaiu the .DEFaULT_GOAL variable is intended for more complex use cases. The make docs say

The .DEFAULT_GOAL variable allows you to discover the current default goal, restart the default goal selection algorithm by clearing its value, or to explicitly set the default goal.

I don't a benefit over just letting make pick the first target as the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The advantage of .DEFAULT_GOAL is that you explicitly set the default goal instead of relying on the structure of the Makefile to define the default goal. For example, in this Makefile we include another file above this declaration, which already defines some goals of which one becomes the default goal.

We can either move the goal we want as default further up in the file (above the include, in this case) or define what we want explicitly (which is what I opted for).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point, I didn't think of the import.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@xperimental xperimental changed the title fix: Default goal in Makefile and git command for reverting changes fix: default goal in makefile and add goal for unit tests May 14, 2024
Copy link
Collaborator

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Copy link

openshift-ci bot commented May 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, xperimental

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-merge-bot openshift-merge-bot bot merged commit b6ed9c5 into rhobs:main May 17, 2024
11 checks passed
@xperimental xperimental deleted the fixes-makefile branch May 17, 2024 13:59
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

2 participants