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

Refactor the Makefile and hack/ scripts #1856

Merged

Conversation

ConnorJC3
Copy link
Contributor

@ConnorJC3 ConnorJC3 commented Dec 7, 2023

Is this a bug fix or adding new feature?

"New Feature"/Refactoring

What is this PR about? / Why do we need it?

This PR contains a significant refactor of the Makefile and e2e testing scripts. Its goals are:

  • Primary Goal: Modularize run.sh to enable separate cluster creation, test running, and cluster deletion. Today, running E2E tests locally requires waiting on the creation and termination of a cluster before and after the run, which takes significant time. Developers should be able to run E2E tests locally against a pre-existing cluster, as well as easily create and delete clusters for testing that follow the standard E2E setup.
  • Primary Goal: Document the available Makefile targets and how to use them. New contributors should not need to go digging in the Makefile to discover how to perform simple tasks such as building the driver or running e2e tests.
  • Secondary Goal: Centralize dependency management. We should have a single place that all local tools are installed - rather than the existing code which has multiple different methods of achieving the same task of installing a binary from GitHub.
  • Secondary Goal: Simply Makefile - many of the existing makefile targets (in particular, the e2e targets) are long, unwieldy, and hard to reason about. Clean up the makefile and hack/ directory as much as possible to remove unneeded complexity.
  • Explicit Non-Goal: Significant changes to how the image is built or how the e2e tests are ran. I do think we should look into improvements in both of these places in the future, but this PR already contains enough changes as is. This PR should not change the behavior of our CI (other than making it run faster and easier to develop for). To this end, eksctl.sh, kops.sh, and metrics.sh are largely or entirely unmodified.
  • Explicit Non-Goal: Changes to Go files - this PR is only about refactoring the testing infrastructure, not the tests themselves.

What testing is done?

Significant local testing as well as CI, see the new docs/makefile.md for the available makefile targets to test against locally

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2023
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 7, 2023
Copy link

github-actions bot commented Dec 7, 2023

Code Coverage Diff

This PR does not change the code coverage

@ConnorJC3 ConnorJC3 force-pushed the optimize-runsh-makefile branch 7 times, most recently from 9caebab to c05edc2 Compare December 7, 2023 22:38
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2023
@AndrewSirenko
Copy link
Contributor

Still working through this PR, but I found this visualization helpful

Pre-PR:
pre-pr-Makefile

Post-PR:
post-pr-Makefile

Generated with vak/makefile2dot: Visualize your Makefile using GraphViz dot utility

@AndrewSirenko
Copy link
Contributor

AndrewSirenko commented Dec 8, 2023

On a high-level, these are great primary and secondary goals, and from a quick-look, the makefile (and file changes at large) are cleaner and more discoverable. I especially appreciate the README as well as the separated commands (e.g e2e/windows, verify/...)

Can we rebase this PR to separate linting, code extraction, filename changes, and variable name changes into separate commits from the main changes to business logic?

Perhaps commits in this order:

  1. Variable name changes
  2. Code extraction
  3. Filename changes
  4. Main Business logic commit (Bonus points if there are separate commits for Each primary/secondary goal)
  5. Linting

Perhaps by running git reset master followed by re-committing changes by hunk/File?

Let me know if parallel programming committing would help in this endeavor. I did something similar for the original versions of #1846 and #1845 before squashing those commits.

Also:

Significant local testing as well as CI, see the new docs/makefile.md for the available makefile targets to test against locally

Means that you tested each makefile target locally and on CI, correct?

@ConnorJC3
Copy link
Contributor Author

Can we rebase this PR to separate linting, code extraction, filename changes, and variable name changes into separate commits from the main changes to business logic?

Is there a significant advantage to doing the large amount work to split this into separate commits? They will ultimately need to be squashed for merge (as all of the commits you listed will be in a broken, partial state) and doing so greatly increases the effort required to update the PR for review comments.

Means that you tested each makefile target locally and on CI, correct?

All the targets were tested locally, and CI runs the targets it already does (make verify, make test, and all the e2e tests)

.dockerignore Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
docs/makefile.md Outdated Show resolved Hide resolved
docs/makefile.md Outdated Show resolved Hide resolved
docs/makefile.md Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@torredil
Copy link
Member

Hitting a quota limit here:

2023-12-11 21:30:31 [✖]  creating OIDC provider: operation error IAM: CreateOpenIDConnectProvider, https response error StatusCode: 409, RequestID: ce25e2a1-fa71-48e4-93a9-0e26ffd23971, LimitExceeded: Cannot exceed quota for OpenIdConnectProvidersPerAccount: 100

cc: @dims

@dims
Copy link
Member

dims commented Dec 11, 2023

@torredil cleaned up a few, please try again?

@torredil
Copy link
Member

/retest

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2023
Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

Great job on this!! Super beneficial to onboarding contributors to the project, as well as maintaining maintainer sanity. Ran a few of the commands, will make sure to run through all of them when I find more time. Will start using this makefile for daily driver development and see what else I find.

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
docs/makefile.md Show resolved Hide resolved
docs/makefile.md Show resolved Hide resolved
docs/makefile.md Show resolved Hide resolved
hack/e2e/config.sh Show resolved Hide resolved
hack/e2e/kops/kops.sh Show resolved Hide resolved
@ConnorJC3 ConnorJC3 force-pushed the optimize-runsh-makefile branch 2 times, most recently from f7f69ff to 5fa99a9 Compare January 8, 2024 16:07
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 8, 2024
Copy link
Contributor

@AndrewSirenko AndrewSirenko left a comment

Choose a reason for hiding this comment

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

LGTM once rebased.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 8, 2024
See the description of PR 1856 for more information about this change

Signed-off-by: Connor Catlett <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024
Copy link
Member

@torredil torredil 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
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 9, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 9, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: torredil

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 Jan 9, 2024
@ConnorJC3
Copy link
Contributor Author

/remove-hold

@k8s-ci-robot k8s-ci-robot removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 9, 2024
@ConnorJC3
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot merged commit fd7dbc2 into kubernetes-sigs:master Jan 9, 2024
19 checks passed
@ConnorJC3 ConnorJC3 mentioned this pull request Feb 14, 2024
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants