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

Developer Experience Improvements #2079

Merged
merged 6 commits into from
Jul 5, 2024

Conversation

ConnorJC3
Copy link
Contributor

@ConnorJC3 ConnorJC3 commented Jul 2, 2024

Is this a bug fix or adding new feature?

Neither - it focuses on improvements to the developer experience.

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

This PR contains several improvements split up by commit:

Only collect metrics in Prow

Metrics collection runs locally by default, but this makes no sense because the metrics will fail to publish anyways. Check for the presence of PROW_JOB_ID and only run metrics in Prow jobs.

Simplify image building

Simplifies handling of image building in the Makefile. Removes some CI-only defaults from the Makefile that are confusing and difficult to use. Renames hack/prow.sh to hack/cloudbuild.sh to clearly indicate what it is used for.

Split driver install/uninstall out to functions; Introduce make cluster/(un)install

Introduces make cluster/install and make cluster/uninstall which can be used to (un)install the driver for local testing. In order to accomplish this, driver installation and uninstallation are split out into a separate function in utils.sh

Cache GOCACHE and GOMODCACHE in Dockerfile

Configures the Dockerfile to cache go modules and build output on supported platforms. In my local testing, this introduced an approximate 30x speedup (from 600 seconds to 20 seconds) when rebuilding the driver image with a small change (such as a dependency upgrade) to go.mod.

Update Makefile documentation and CONTRIBUTING.md

Makefile documentation was somewhat out of date, cleans it up. Also moves the quickstart guide to CONTRIBUTING.md and cleans up outdated information in CONTRIBUTING.md.

Cleanup non-external e2e test handling

Cleans up the logic of non-external tests, fixing a bug that causes the tests to fail to run when run from inside a home directory that is a symlink.

What testing is done?

CI/Manual

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 2, 2024
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 2, 2024
Copy link

github-actions bot commented Jul 2, 2024

Code Coverage Diff

This PR does not change the code coverage

Signed-off-by: Connor Catlett <conncatl@amazon.com>
@AndrewSirenko
Copy link
Contributor

AndrewSirenko commented Jul 3, 2024

Thanks for these amazing devloop + new contributor improvements.

TIL: cache mounts... (Thank you for this...)

xref: #1929 (Because this PR resolves checklist items 1,2, and 3)

Working my way through testing the PR, eta end of week.

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.

Thank you for this. It erases so many minutes from creating/reviewing PRs.

Left mostly non-blocking comments except for adding a pre-requisite for having a private ECR repository named aws-ebs-csi-driver (or overriding that default)

Besides ^^, I went through the new contributing guide steps and it all worked for me :)

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
docs/makefile.md Outdated
@@ -20,15 +20,17 @@ All other tools are downloaded for you at runtime.

This guide demonstrates the basic workflow for developing for the EBS CSI Driver. More detailed documentation of the available `make` targets is available below.

### 1. Local development for the EBS CSI Driver
### 1. Local development
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we directly reference these same steps in contributing-guide instead of having the same information in two places?

Would also shorten this document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, I swear I did that but I think I reverted this file on accident when messing with git rebasing - will fix in next rev

docs/makefile.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2024
Signed-off-by: Connor Catlett <conncatl@amazon.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2024
@AndrewSirenko
Copy link
Contributor

/lgtm

Will approve once devloop with EKS confirmed works.

…er/(un)install

Signed-off-by: Connor Catlett <conncatl@amazon.com>
Signed-off-by: Connor Catlett <conncatl@amazon.com>
Signed-off-by: Connor Catlett <conncatl@amazon.com>
Signed-off-by: Connor Catlett <conncatl@amazon.com>
@AndrewSirenko
Copy link
Contributor

/lgtm
/approve

@ConnorJC3
Copy link
Contributor Author

/retest

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AndrewSirenko

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jul 5, 2024
@AndrewSirenko
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2024
@k8s-ci-robot k8s-ci-robot merged commit 3ea9041 into kubernetes-sigs:master Jul 5, 2024
19 checks passed
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants