fix: Add release-tools subtree

[animeshk08]

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 test
/kind failing-test

/kind flake

/kind feature
What this PR does / why we need it:
Adds release tool subtree to the repository. This make the repository template uniform with other repositories within Kubernetes-CSI. Also post-csi-driver-smb-push-images fail can be fixed
Which issue(s) this PR fixes:

Fixes #

Requirements:

• uses conventional commit messages
• includes documentation
• adds unit tests
• tested upgrade from previous version

Special notes for your reviewer:

Release note:

fix: Add release-tools subtree

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: animeshk08
To complete the pull request process, please assign andyzhangx
You can assign the PR to them by writing /assign @andyzhangx in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[animeshk08]

Followed instructions from here: https://github.com/kubernetes-csi/csi-release-tools#sharing-and-updating
Most of the repositories of Kubernetes-csi are based on this template. Hence, it made sense to structure this repository in the same manner.

This PR should also get rid of the following job-failure: post-csi-driver-smb-push-images

NOTE: The commits of the subtree have been squashed.

[animeshk08]

One issue remains in this PR:
The following section states to add a symlink of travis.yaml. However, I am unsure if it is really needed and how to combine it with the current travis configurations.

[coveralls]

Pull Request Test Coverage Report for Build 194508756

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 88.317%

---

Totals
Change from base Build 189729996:	0.0%
Covered Lines:	635
Relevant Lines:	719

---

💛 - Coveralls

[animeshk08]

This will build as well as run the tests. I am not sure if this will be redundant(Are tests being run twice?). If it is redundant this line can be replaced with all: build

[pohly]

I think the usual expectation is that a plain "make" will just build, i.e. use all: build here.

[animeshk08]

Thank you for the info. I will update the PR :)

[msau42]

Now that we have cloudbuild integration to do multi-arch image building, we no longer need travis CI and can rely entirely on prow jobs to build and test. cc @pohly

[andyzhangx]

Now that we have cloudbuild integration to do multi-arch image building, we no longer need travis CI and can rely entirely on prow jobs to build and test. cc @pohly

let's keep travis for a while until github action has all the test coverage, here is the golint error reported from travis:

Cannot find golangci-lint. Installing golangci-lint...
golangci/golangci-lint info checking GitHub for tag 'v1.24.0'
golangci/golangci-lint info found version: 1.24.0 for v1.24.0/linux/amd64
golangci/golangci-lint info installed /home/travis/gopath/bin/golangci-lint
release-tools/filter-junit.go:17:1: package comment should not have leading space (golint)
/*
^
Makefile:55: recipe for target 'verify' failed
make: *** [verify] Error 1
The command "make verify" exited with 2.

[animeshk08]

I have added a PR to fix the golint error in the source repository: kubernetes-csi/csi-release-tools#92

[msau42]

@andyzhangx what I mean is that the prow job also has coverage in terms of build, fmt, and unit tests

[pohly]

Squashing the commits in release-tools breaks future updates:

$ hub checkout https://github.com/kubernetes-csi/csi-driver-smb/pull/28
...
$ git subtree pull --prefix=release-tools https://github.com/kubernetes-csi/csi-release-tools.git master
warning: no common commits
remote: Enumerating objects: 64, done.
remote: Counting objects: 100% (64/64), done.
remote: Compressing objects: 100% (48/48), done.
remote: Total 411 (delta 29), reused 37 (delta 16), pack-reused 347
Receiving objects: 100% (411/411), 200.51 KiB | 1.04 MiB/s, done.
Resolving deltas: 100% (216/216), done.
From https://github.com/kubernetes-csi/csi-release-tools
 * branch              master     -> FETCH_HEAD
fatal: refusing to merge unrelated histories

Please import csi-release-tools with git subtree add as described in the README.md.

[animeshk08]

Hi, @pohly I was successfully able to pull future updates by squashing the pull as well.

git subtree pull --prefix=release-tools https://github.com/kubernetes-csi/csi-release-tools.git master --squash

Adding the subtree as mentioned in the README as such will show unnecessary commits in this repo's tree. Let me know how to move forward.

Also now we have gofmt errors in filter-junit. I have created a PR for the same. Please have a look: kubernetes-csi/csi-release-tools#93
cc @andyzhangx

[pohly]

I was successfully able to pull future updates by squashing the pull as well.

Okay, that works.

Adding the subtree as mentioned in the README as such will show unnecessary commits in this repo's tree.

It's debatable whether those commits are unnecessary. With squashing, it is impossible to tell which changes from csi-release-tools are in a PR and (later) in the repo. For comparison, this is what an update PR looks like when keeping history: kubernetes-csi/csi-proxy#57

IMHO consistency is important. @msau42 what do you think?

One possible alternative to subtree is importing csi-release-tools as submodule. If Prow always checks out with submodules (to be checked - I don't know whether it does), then we still have the full set of files needed during the job. But then developers will have to adapt their workflow accordingly. I did not investigated that earlier because I had the impression that TravisCI always wants a .travis.yml in the repo itself; this might have been wrong.

[animeshk08]

Hi, @msau42 @pohly any updates on this?

[msau42]

I am fine with squashing the commits from csi-release-tools. In the other repos, it does add a bit of noise to the commit history. The commit here says which commit we pulled from csi-release-tools so if we really needed to backtrack we could. @animeshk08 can you also update the README in csi-release-tools with your squash steps? I think we can start to adapt this in the other sidecar repos too.

[pohly]

@animeshk08 can you also update the README in csi-release-tools with your squash steps?

And please also adapt the script in kubernetes-csi/csi-release-tools#7 (comment)

It will break once repos start to squash commits, in which case individual repo owners will have to manually update csi-release-tools. So far, I was creating PRs for them with that script.

[pohly]

Squashing is IMHO worse than using a git submodule. Shall we switch to that instead?

[animeshk08]

It will break once repos start to squash commits, in which case individual repo owners will have to manually update csi-release-tools. So far, I was creating PRs for them with that script.

Hi @pohly. Your concerns are valid. The script will have to be modified in case squashing becomes popular among all the repos. Also, I understand that having a submodule will not increase the size of the repos as compared to having a subtree.

If it works I also agree that submodule will be a better choice(I am not well versed with the intricies of prow). Should I create a PR specific to the current repo to see if any problems are faced?

[pohly]

Should I create a PR specific to the current repo to see if any problems are faced?

That's a worthwhile test. There is an image build job for it (https://github.com/kubernetes/test-infra/blob/326537faeda35c19f725d5daba46b7650317aefe/config/jobs/image-pushing/k8s-staging-sig-storage.yaml#L91) and runs for it can be found through the dashboard (https://testgrid.k8s.io/sig-storage-image-build#post-csi-driver-smb-push-images).

We'll have to merge the PR first, but as that can only improve the situation, that's okay.

[pohly]

@animeshk08: we discussed this today in the CSI standup meeting and the conclusion was that a) sub-modules might be too confusing because developers would have to remember to update also the sub-module b) that squashing is probably better than importing the full commit history.

However, for that to work in practice we need an update for the script such that it handles squashing and still is reporting the full list of commits that are added as part of a PR, both in the commit message and the PR message. See kubernetes-csi/external-health-monitor#14 for an example how that looks at the moment. In the future, commit numbers have to become links to the csi-release-tools repo because the commit has no meaning anymore in the other repos.

[animeshk08]

Thank you for the update @pohly! I will try to modify the script.

[animeshk08]

Hi @pohly. Sorry for the delay.

I have spent some time trying to figure out a way to squash the commits and still follow the PR format currently being used. From my understanding, there is no direct way to implement this. However, if some flexibility is adapted we can achieve something similar as described below.

I understand that the script currently only pulls the changes and doesn't add a new subtree. The new adaption will also have this short coming.

After pulling new changes the tree will look something like below:

Merge: a7ebb2f4 345969fd
Author: Animesh Kumar <[email protected]>
Date:   Wed Jul 8 04:51:49 2020 +0530

    Merge commit '345969fdb33e017ffaa5d72ac37f4ea420f56970' into prow-update-master

commit 345969fdb33e017ffaa5d72ac37f4ea420f56970
Author: Animesh Kumar <[email protected]>
Date:   Wed Jul 8 04:51:49 2020 +0530

    Squashed 'release-tools/' changes from f5a42037..41ec6d15
    
    41ec6d15 Merge pull request #93 from animeshk08/patch-1
    5a54e67d filter-junit: Fix gofmt error
    0676fcbd Merge pull request #92 from animeshk08/patch-1
    36ea4ffa filter-junit: Fix golint error
    
    git-subtree-dir: release-tools
    git-subtree-split: 41ec6d15300b3a199ee217561698869d30fcf7bb

As you can see the changes even though squashed are included in the commit message.

If we do not rewrite the commit message anymore we can utilise something like above.
Now to get this data inside the comment of a PR we just include the commit message of the last non-merge commit.

The modified script will look something like below:

#!/bin/sh -x

die () {
    echo >&2 "\nERROR: $@\n"
    exit 1
}

while read repo branches; do
    if [ "$repo" != "#" ]; then
    (
        cd $repo || die "$repo: does not exit"
        git fetch origin || die "$repo: git fetch"
        for i in $branches; do
            git checkout -B prow-update-$i origin/$i || die "$repo:$i checkout"
            git subtree pull --prefix=release-tools https://github.com/kubernetes-csi/csi-release-tools.git master --squash|| die "$repo:$i update release tools"
            changes=$(git log --no-merges --format=%B -n 1 HEAD)
            if [ "$changes" ]; then
                release-tools/update-vendor.sh || die "update vendor"
                git add vendor
                git commit --amend --no-edit
                make test || die "$repo:$i make test"
                git push pohly prow-update-$i || die "pohly:prow-update-$i push failed - probably there is already an unmerged branch and pending PR"
                hub pull-request -F - -b kubernetes-csi/$repo:$i -h pohly/$repo:prow-update-$i <<EOF
$i: update release-tools

Commit summary:
$changes

\`\`\`release-note
NONE
\`\`\`
EOF
                if [ "$?" -ne 0 ]; then
                    die "$repo:$i pull-request"
                fi
            fi
        done
    ) || die "failed"
    fi
done <<EOF
csi-driver-host-path master
csi-driver-iscsi master
csi-driver-nfs master
csi-lib-utils master
csi-proxy master
csi-test master
external-attacher master
external-health-monitor master
external-provisioner master
external-resizer master
external-snapshotter master
livenessprobe master
node-driver-registrar master
EOF

I spent quite some time trying other methods like finding diff/log between branches and even remotes. But nothing was successful.

Please, let me know what are your views on this.

\cc @andyzhangx @msau42

EDIT:
PR comment looks like below:

Commit summary:
Squashed 'release-tools/' changes from f5a42037..41ec6d15

41ec6d15 Merge pull request #93 from animeshk08/patch-1
5a54e67d filter-junit: Fix gofmt error
0676fcbd Merge pull request #92 from animeshk08/patch-1
36ea4ffa filter-junit: Fix golint error

git-subtree-dir: release-tools
git-subtree-split: 41ec6d15300b3a199ee217561698869d30fcf7bb


\`\`\`release-note
NONE
\`\`\`

[pohly]

@animeshk08: looks like you need to update the PR to include kubernetes-csi/csi-release-tools@0345a83

[pohly]

/lgtm

[animeshk08]

I have updated the PR. Thank you for all the help @pohly.

\cc @andyzhangx

[k8s-ci-robot]

@animeshk08: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-csi-driver-smb-verify	2bfb646	link	/test pull-csi-driver-smb-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[animeshk08]

i believe the verify test and build test are failing as there is missing boiler plate text in some of the files in release-tools like cloudbuild.sh.

The boiler plate text(License) can should be of the following format:

# Copyright YEAR The Kubernetes Authors.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
#     http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

Can I send a PR to csi-release-tools to add this @pohly?

[pohly]

Can I send a PR to csi-release-tools to add this @pohly?

Sure.

However, there is a more fundamental issue: csi-driver-smb seems to have additional tests that aren't used by other repos and then expects code imported from elsewhere to pass those tests. This needs to be changed, either by:

• adding those tests to csi-release-tools and then running them also there and in other repos using csi-release-tools (consistent code quality!)
• excluding release-tools from the tests that are only used in csi-driver-smb

[animeshk08]

However, there is a more fundamental issue: csi-driver-smb seems to have additional tests that aren't used by other repos and then expects code imported from elsewhere to pass those tests.

The concern is genuine. This is not the correct way to move forward to fix an error in the upstream repo after every update of csi-release-tools.

I think the choice of the path from here should be decided after some discussion. I believe the test in csi-driver-smb have proved to be useful. The maintainers of other projects can have a look at these tests, and if they decide to adopt it we can include it to other projects as well(maybe via release-tools itself). This will help in improving the project quality and consistency.

If the decision is otherwise we can remove the test for specifically for release-tools.
WDYT? @andyzhangx @pohly

[andyzhangx]

@animeshk08 I think we can exclude release-tools from the tests that are only used in csi-driver-smb thanks.

[pohly]

IMHO the header check is useful.

I'm less convinced about golint. The upstream project explicitly warns against expecting code to pass golint without warnings (https://github.com/golang/lint#purpose - "In short, this tool is not, and will never be, trustworthy enough for its suggestions to be enforced automatically, for example as part of a build process."). Enforcing this via a check will put us into a position where we have to make code changes even when they don't make sense. Changing the quoting style for example (kubernetes-csi/csi-release-tools@36ea4ff) did not make much sense as far as I'm concerned.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.