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

Import the kubernetes-csi/csi-release-tools subtree. #586

Merged
merged 2 commits into from
Aug 25, 2020
Merged

Import the kubernetes-csi/csi-release-tools subtree. #586

merged 2 commits into from
Aug 25, 2020

Conversation

jeremyje
Copy link
Contributor

@jeremyje jeremyje commented Aug 21, 2020

What type of PR is this?

/kind feature

What this PR does / why we need it:
Imports the kubernetes-csi/csi-release-tools repository to align the build with other CSI projects.

This PR simply adds the repository, it does not modify the current Makefile, that'll be done in a future PR.

Which issue(s) this PR fixes:

#585

Special notes for your reviewer:
I have done limited testing some things are currently broken but that's expected. Notably the e2e tests.

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 21, 2020
@k8s-ci-robot
Copy link
Contributor

@jeremyje: The label(s) kind/build cannot be applied, because the repository doesn't have them

In response to this:

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

/kind build
/kind cleanup

What this PR does / why we need it:
Replace the custom Makefile with the imported kubernetes-csi/csi-release-tools repository.

This simplifies the build system and aligns with the greater set of CSI drivers.

This PR also resolves most of the escape issues found via $PWD/release-tools/verify-shellcheck.sh since the instructions recommended that.

Which issue(s) this PR fixes:

Fixes #585

Special notes for your reviewer:
I have done limited testing some things are currently broken but that's expected. Notably the e2e tests.

Does this PR introduce a user-facing change?:

NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 21, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @jeremyje!

It looks like this is your first PR to kubernetes-sigs/gcp-compute-persistent-disk-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/gcp-compute-persistent-disk-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

Hi @jeremyje. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 21, 2020
@jeremyje jeremyje changed the title Releasetools Replace Custom Makefile with csi-release-tools. Aug 21, 2020
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 21, 2020
deploy/common.sh Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@msau42
Copy link
Contributor

msau42 commented Aug 21, 2020

/ok-to-test
Can we also squash all of the commits into one?

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 21, 2020
Copy link
Contributor

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

Some of the content in csi-release-tools is very specific to the way we test csi-sidecars and do sidecar releases, and are not applicable here, such as bringing up a kind cluster for CI, and deploying the hostpath driver.

Will it be confusing if we include those irrelevant parts here? We are mainly interested in the build rules and not much else right?

@jingxu97
Copy link
Contributor

Some of the content in csi-release-tools is very specific to the way we test csi-sidecars and do sidecar releases, and are not applicable here, such as bringing up a kind cluster for CI, and deploying the hostpath driver.

Will it be confusing if we include those irrelevant parts here? We are mainly interested in the build rules and not much else right?

bringing up a kind cluster is also useful for linux case? I feel csi-release-tool is gives a very organized way for building and releasing part. So we can use this consistently for all csi related components.

@msau42
Copy link
Contributor

msau42 commented Aug 21, 2020

I don't think we can test pd driver in a kind cluster

@msau42
Copy link
Contributor

msau42 commented Aug 21, 2020

I agree reusing the make rules is useful, but I don't know about the rest, like prow.sh and sidecar_release_process

@jingxu97
Copy link
Contributor

I agree reusing the make rules is useful, but I don't know about the rest, like prow.sh and sidecar_release_process

another option is take the part we need from release-tool repo directly instead of vendering it if possible?

@jeremyje
Copy link
Contributor Author

I've split the prep changes of this PR to the following below. That should be less disruptive.

#588

#587

@jeremyje
Copy link
Contributor Author

I agree reusing the make rules is useful, but I don't know about the rest, like prow.sh and sidecar_release_process

another option is take the part we need from release-tool repo directly instead of vendering it if possible?

The csi-release-tools codebase has specific instructions on how to import it into a codebase. https://github.com/kubernetes-csi/csi-release-tools#sharing-and-updating
My concern here is deviating away from this will cause a support burden for this repository in the future. They even supply a tool to verify diffs, https://github.com/kubernetes-csi/csi-release-tools/blob/master/verify-subtree.sh.

@msau42
Copy link
Contributor

msau42 commented Aug 21, 2020

I find the flood of commits very distracting as it becomes harder to tell which commits are actually relevant to the driver. We're trying to move towards a model where all the commits imported from csi-release-tools are squashed, but the PR description contains all the commits that were imported

@jingxu97
Copy link
Contributor

I agree reusing the make rules is useful, but I don't know about the rest, like prow.sh and sidecar_release_process

another option is take the part we need from release-tool repo directly instead of vendering it if possible?

The csi-release-tools codebase has specific instructions on how to import it into a codebase. https://github.com/kubernetes-csi/csi-release-tools#sharing-and-updating
My concern here is deviating away from this will cause a support burden for this repository in the future. They even supply a tool to verify diffs, https://github.com/kubernetes-csi/csi-release-tools/blob/master/verify-subtree.sh.

I agree. I checked release tool page again, I think with recent improvement in the tool such as multi-ach build and release, it is very useful and convenient to use. The tool itself seem quite light weight. So vendoring the whole tool seems ok to me.

@jeremyje
Copy link
Contributor Author

I find the flood of commits very distracting as it becomes harder to tell which commits are actually relevant to the driver. We're trying to move towards a model where all the commits imported from csi-release-tools are squashed, but the PR description contains all the commits that were imported

This is great! I'll look into this instead. For now I think based on the feedback given here and offline that I'll scale this change back to simply adding the release-tools directory. There's other PRs out for the shellcheck fixes and the cmd/ move.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2020
@jeremyje jeremyje changed the title Replace Custom Makefile with csi-release-tools. Import the kubernetes-csi/csi-release-tools subtree. Aug 21, 2020
@jeremyje
Copy link
Contributor Author

@msau42 I've changed this PR to simply import the csi-release-tools subtree. The Makefile is untouched so there should be no changes required for CI.

@msau42
Copy link
Contributor

msau42 commented Aug 25, 2020

/lgtm
/approve
Thanks!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyje, msau42

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 Aug 25, 2020
*/

/*
This command filters a JUnit file such that only tests with a name
Copy link
Contributor

Choose a reason for hiding this comment

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

@mattcary can you changes to combine the junit files be refactored now that it's actually imported in the repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

#591

Great point. It may be a little complicated as IIRC I made some changes to the junit-filter code which was built only as a command line tool and not a go library. But we should be able to dedup something, maybe by also extending the refactoring to the release tools.

@k8s-ci-robot k8s-ci-robot merged commit 11fbbea into kubernetes-sigs:master Aug 25, 2020
@jeremyje jeremyje deleted the releasetools branch August 25, 2020 18:03
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. 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