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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃尡 consolidate test/infrastructure/docker/Makefile to Makefile #6443

Merged

Conversation

chrischdi
Copy link
Member

@chrischdi chrischdi commented Apr 26, 2022

What this PR does / why we need it:

Removes all targets in test/infrastructure/docker/Makefile and replaces them by equivalents in Makefile.

Reduces the effort of maintaining and syncing to the additional Makefile.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):

Fixes #6445

TODO:

  • squash commits, for now 2 commits to help on reviews

Additional info:

Detailed list of migrated targets:

old makefile target new makefile target changes
test test-capd -
test-verbose test-capd-verbose allows to include additional TEST_ARGS
test-junit test-capd-junit -
manager manager-capd output to BIN_DIR instead of test/infrastructure/docker/$(BIN_DIR)
generate generate integrated into core generate command
generate-go generate-go-deepcopy-capd splitted into two targets (as handled in core)
generate-go generate-go-conversion-capd -
generate-manifests generate-manifests-capd -
docker-pull-prerequisites docker-pull-prerequisites did already exist
docker-build docker-capd-build -
docker-push docker-capd-push -
docker-build-all docker-capd-build-all -
docker-build-% docker-capd-build-% -
docker-push-all docker-capd-push-all -
docker-push-% docker-capd-push-% -
docker-push-manifest docker-capd-push-manifest -
release release did already use old capd release target, does the same via release-manifest-dev modification
manifest-modification manifest-modification capd parts got integrated to main Makefile function
release-manifests release-manifests-dev directly outputs now to RELEASE_DIR, not anymore to test/infrastructure/docker/out
release-staging release-capd-staging may also include core release artifacts, is was always used together
release-staging-nigtly release-capd-staging-nigtly may also include core release artifacts, is was always used together
release-alias-tag release-capd-alias-tag -
clean-bin clean-bin does not cleanup test/infrasturcture/docker/bin however this directory is not in-use anymore.
clean-release clean-release does not cleanup test/infrasturcture/docker/bin however this directory is not in-use anymore.
verify verify / verify-capd verify executes verify-capd and verify-gen which results into still testing the same.
verify-gen verify-gen did already exist and compatible due to generate target to also include capd generate targets.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 26, 2022
@chrischdi chrischdi changed the title 馃尡 [WIP] consolidate test/infrastructure/docker/Makefile to Makefile 馃尡 [WIP] consolidate test/infrastructure/docker/Makefile to Makefile Apr 26, 2022
@chrischdi chrischdi changed the title 馃尡 [WIP] consolidate test/infrastructure/docker/Makefile to Makefile 馃尡 [WIP] consolidate test/infrastructure/docker/Makefile to Makefile Apr 26, 2022
@chrischdi
Copy link
Member Author

chrischdi commented Apr 26, 2022

/retitle 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile

Open question from my side:

  • For now I separated the capd build & push targets to not be included in docker-build-all (instead docker-capd-build-all) and docker-push-all (instead docker-capd-push-all). I can think of users want to push core capi but don't care about capd.

I did not do this for generate because I thought that's not worth it.

@k8s-ci-robot k8s-ci-robot changed the title 馃尡 [WIP] consolidate test/infrastructure/docker/Makefile to Makefile 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile Apr 26, 2022
@chrischdi chrischdi changed the title 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile Apr 27, 2022
@chrischdi chrischdi changed the title 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile Apr 27, 2022
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 13, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 16, 2022
@chrischdi
Copy link
Member Author

rebased :-)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 4, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022
@chrischdi
Copy link
Member Author

Rebased the PR

/test all

cc @sbueringer

@chrischdi chrischdi changed the title 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile Jun 9, 2022
@chrischdi chrischdi changed the title 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile 馃尡 consolidate test/infrastructure/docker/Makefile to Makefile Jun 9, 2022
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Very nice, a few nits, but otherwise looks good

docs/book/src/developer/providers/v1.1-to-v1.2.md Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 9, 2022
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 10, 2022
@chrischdi
Copy link
Member Author

cc @sbueringer : addressed comments and rebased again 馃榾

Makefile Show resolved Hide resolved
@sbueringer
Copy link
Member

@chrischdi One comment which should hopefully fix the verify job, otherwise lgtm (feel free to squash)

@sbueringer
Copy link
Member

Thank you very much!
We wanted to get rid of that Makefile for a long time :)

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 10, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 10, 2022
@k8s-ci-robot k8s-ci-robot merged commit 88dc60e into kubernetes-sigs:main Jun 10, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Jun 10, 2022
@chrischdi chrischdi deleted the pr-makefile-refactoring branch June 30, 2022 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Makefile consilidation to improve maintenance and developer experience
3 participants