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

added prow job for depstat #22137

Merged
merged 12 commits into from
May 14, 2021
Merged

added prow job for depstat #22137

merged 12 commits into from
May 14, 2021

Conversation

RinkiyaKeDad
Copy link
Member

Signed-off-by: RinkiyaKeDad arshsharma461@gmail.com

  • In the last sig-testing meeting it was decided that the best way to more forward with the integration of depstat with k/k would be to have a separate prow job.
  • In the job we generate the stats for a PR and then git checkout to the base SHA (main branch of k/k) and then generate the stats for that and then finally diff the two results.
  • I've used the golang image for the prow job and installed git as part of the bash script (which is passed inline).

Huge thanks to @spiffxp for helping out with this!

Link to the relevant slack discussion.

cc @dims

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 11, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @RinkiyaKeDad!

It looks like this is your first PR to kubernetes/test-infra 🎉. 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/test-infra 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 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 May 11, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @RinkiyaKeDad. Thanks for your PR.

I'm waiting for a kubernetes 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 area/config Issues or PRs related to code in /config area/conformance Issues or PRs related to kubernetes conformance tests area/jobs sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 11, 2021
@RinkiyaKeDad
Copy link
Member Author

/assign @spiffxp

BASE_SHA="${PULL_BASE_SHA:-""}"; ARTIFACTS="${ARTIFACTS:-tmp/artifacts}";
mkdir -p "${ARTIFACTS}";
export GO111MODULE=on;
go get github.com/kubernetes-sigs/depstat;
Copy link
Member

Choose a reason for hiding this comment

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

go get or go install?

Copy link
Member

Choose a reason for hiding this comment

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

also doesn't doing a go get in this directory impact the go.mod/sum of kubernetes itself? (leading to bad results)

Copy link
Member

Choose a reason for hiding this comment

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

were you able to test this locally? you can use docker run with the golang image drop into shell and simulate the script. you can volume mount kubernetes/kubernetes as well.

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@RinkiyaKeDad RinkiyaKeDad requested a review from dims May 12, 2021 13:01
@dims
Copy link
Member

dims commented May 12, 2021

/ok-to-test

@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. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 12, 2021
@dims
Copy link
Member

dims commented May 12, 2021

@RinkiyaKeDad you can run shell check on the snippet. see what popped when i tried it. (shellcheck.net). let's fix at least the SC2086's

image

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@RinkiyaKeDad
Copy link
Member Author

@dims fixed all the ones shown except this:

image

AFAIK we need to add a comment like #!/bin/bash to fix it right? Do we do that when we specify the script inline like we're doing here?

containers:
- image: golang
command: ["/bin/bash"]
args: ["-c", "set -euo;
Copy link
Member Author

Choose a reason for hiding this comment

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

I think one of the tests is failing because it expects a ] at the end of this line. So should I have the entire thing in one line to fix this? '-'

@dims @spiffxp

Copy link
Member

Choose a reason for hiding this comment

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

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@BobyMCbobs
Copy link
Member

/uncc @BobyMCbobs

@k8s-ci-robot k8s-ci-robot removed the request for review from BobyMCbobs May 13, 2021 06:06
git checkout -b base "${BASE_SHA}" \
write_report > "${ARTIFACTS}/stats-base.json" \
diff_reports "${ARTIFACTS}"/stats-base.json "${ARTIFACTS}"/stats.json \
fi \
Copy link
Member

Choose a reason for hiding this comment

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

remove \ at the end of the last line.

Copy link
Member

Choose a reason for hiding this comment

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

there are other issues as well. here's how you test

image

Copy link
Member

Choose a reason for hiding this comment

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

you may need to separate lines using ;

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@RinkiyaKeDad
Copy link
Member Author

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 13, 2021
Comment on lines 15 to 16
BASE_SHA="${PULL_BASE_SHA:-"09268c16853b233ebaedcd6a877eac23690b5190"}"; \
ARTIFACTS="${ARTIFACTS:-/tmp/artifacts}"; \
Copy link
Member Author

Choose a reason for hiding this comment

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

@dims if I exclude these two lines and run bash -c "<rest of the script>" it works. With them I get this error:

mkdir: missing operand
Try 'mkdir --help' for more information.

This is because these variables don't get set when the entire script is passed to bash -c.

What I can't seem to understand is why this is happening because individually running these two commands and then passing the rest of the script works.

P.S. I'll remove the hardcoded SHA before we merge.

Copy link
Member

Choose a reason for hiding this comment

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

try this, note the quotes.

          BASE_SHA=${PULL_BASE_SHA:-"09268c16853b233ebaedcd6a877eac23690b5190"}; \
          ARTIFACTS=${ARTIFACTS:-/tmp/artifacts}; \

Copy link
Member Author

Choose a reason for hiding this comment

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

Still not working 😞

Copy link
Member

Choose a reason for hiding this comment

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

try bash -c '' (try single quote there with the change above too)

Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't work too :(

Trying something simpler like: bash -c 'ARTIFACTS="/tmp/artifacts";' is also not working.

@RinkiyaKeDad RinkiyaKeDad requested a review from dims May 13, 2021 13:31
@RinkiyaKeDad
Copy link
Member Author

RinkiyaKeDad commented May 13, 2021

Also the only test failing says:

FAIL: TestKubernetesProwInstanceJobsMustHaveMatchingTestgridEntries (0.00s)
    config_test.go:495: Job kubernetes-depstat does not have a matching testgrid testgroup

Can you please point me to where I have to add this "testgrid"?

EDIT: I guess somewhere here: https://github.com/kubernetes/test-infra/tree/master/testgrid, right?

@dims
Copy link
Member

dims commented May 13, 2021

not sure what you are doing @RinkiyaKeDad :)

image

@dims
Copy link
Member

dims commented May 13, 2021

good news is .. what you have in there works fine :) I had to fix the default for BASE_SHA

[1692161:1692160 - 0:1938] 05:58:25 [dims@bigbox:/dev/pts/0 +1] ~/go/src/k8s.io/kubernetes
$ docker run -it -v $HOME/go/src/k8s.io/kubernetes:/go/src/k8s.io/kubernetes golang bash
root@17b350784137:/go# bash -c  'set -euo; \
>           BASE_SHA="${PULL_BASE_SHA:-"HEAD~1"}"; \
>           ARTIFACTS="${ARTIFACTS:-/tmp/artifacts}"; \
>           mkdir -p "${ARTIFACTS}"; \
>           cd "$ARTIFACTS"; \
>           go install github.com/kubernetes-sigs/depstat@latest; \
>           cd /go/src/k8s.io/kubernetes; \
>           function write_report() { depstat stats --json; }; \
>           write_report > "${ARTIFACTS}/stats.json"; \
>           if [ -n "${BASE_SHA}" ]; then \
>           git checkout -b base "${BASE_SHA}"; \
>           write_report > "${ARTIFACTS}/stats-base.json"; \
>           diff "${ARTIFACTS}"/stats-base.json "${ARTIFACTS}"/stats.json; \
>           fi;'
allexport      	off
braceexpand    	on
emacs          	off
errexit        	on
errtrace       	off
functrace      	off
hashall        	on
histexpand     	off
history        	off
ignoreeof      	off
interactive-comments	on
keyword        	off
monitor        	off
noclobber      	off
noexec         	off
noglob         	off
nolog          	off
notify         	off
nounset        	on
onecmd         	off
physical       	off
pipefail       	off
posix          	off
privileged     	off
verbose        	off
vi             	off
xtrace         	off
go: downloading github.com/kubernetes-sigs/depstat v0.5.3
go: downloading github.com/spf13/cobra v1.1.1
go: downloading github.com/spf13/pflag v1.0.5
Switched to a new branch 'base'
root@17b350784137:/go# ls /tmp/artifacts
stats-base.json  stats.json
root@17b350784137:/go# cat /tmp/artifacts/stats.json
{
	"totalDependencies": 395,
	"maxDepthOfDependencies": 32,
	"transitiveDependencies": 266
}
root@17b350784137:/go#

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@dims
Copy link
Member

dims commented May 14, 2021

@RinkiyaKeDad ping me when this is green please

Signed-off-by: RinkiyaKeDad <arshsharma461@gmail.com>
@dims
Copy link
Member

dims commented May 14, 2021

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, RinkiyaKeDad

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 May 14, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8794177 into kubernetes:master May 14, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 14, 2021
@k8s-ci-robot
Copy link
Contributor

@RinkiyaKeDad: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

  • key kubernetes-depstat.yaml using file config/jobs/kubernetes/sig-arch/kubernetes-depstat.yaml

In response to this:

Signed-off-by: RinkiyaKeDad arshsharma461@gmail.com

  • In the last sig-testing meeting it was decided that the best way to more forward with the integration of depstat with k/k would be to have a separate prow job.
  • In the job we generate the stats for a PR and then git checkout to the base SHA (main branch of k/k) and then generate the stats for that and then finally diff the two results.
  • I've used the golang image for the prow job and installed git as part of the bash script (which is passed inline).

Huge thanks to @spiffxp for helping out with this!

Link to the relevant slack discussion.

cc @dims

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.

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. area/config Issues or PRs related to code in /config area/conformance Issues or PRs related to kubernetes conformance tests area/jobs 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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants