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.

Sign up for GitHub

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

Jump to bottom

Kubetest Version Addition #21276

Merged
merged 1 commit into from
Mar 13, 2021
Merged

Kubetest Version Addition #21276

merged 1 commit into from
Mar 13, 2021

Conversation

supriya-premkumar
Copy link
Contributor

@supriya-premkumar supriya-premkumar commented Mar 10, 2021 •
edited by spiffxp
Loading

Changes:

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 10, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @supriya-premkumar. 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/images area/kubetest area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Mar 10, 2021
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Mar 10, 2021
@supriya-premkumar
Copy link
Contributor Author

store version info as an artifact

@spiffxp @dims I was not able to figure artifact stuff from looking at the code.

Is metadata.json and artifacts different? Appreciate any pointers.

@supriya-premkumar supriya-premkumar force-pushed the kubetest-version branch 2 times, most recently from e3de79e to 4259c99 Compare March 10, 2021 05:12
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Mar 10, 2021
@supriya-premkumar
Copy link
Contributor Author

/assign @dims @spiffxp @BenTheElder

@BenTheElder
Copy link
Member

/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 Mar 10, 2021
amwat
amwat reviewed Mar 10, 2021
View reviewed changes
Copy link
Contributor

@amwat amwat 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 adding this!

just a couple of minor comments, but the approach lgtm!

images/kubekins-e2e/cloudbuild.yaml Outdated Show resolved Hide resolved
kubetest/main.go Outdated Show resolved Hide resolved
@supriya-premkumar supriya-premkumar force-pushed the kubetest-version branch 3 times, most recently from e4e1ccd to 183d223 Compare March 12, 2021 05:01
amwat
amwat reviewed Mar 12, 2021
View reviewed changes
Copy link
Contributor

@amwat amwat left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this change!
/approve

just a final comment before lgtm

kubetest/main.go Outdated
@@ -289,6 +293,11 @@ func main() {
log.Fatalf("Flags validation failed. err: %v", err)
}

if o.version {
log.Printf("kubetest version: %s", gitTag)
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: since this is the expected output of the command this should be written to stdout :)
(and a newline) e.g. for something likekubetest --version 2>/dev/null

maybe fmt.Printf("kubetest version: %s\n", gitTag) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

Copy link
Contributor

Choose a reason for hiding this comment

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

might've missed log vs fmt

log defaults to stderr, fmt defaults to stdout.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2021
@supriya-premkumar
Kubetest Version Addition
b476601 
- Adds version flag for kubetest
- Adds kubetest version in metadata.json
- Fixes 20886
spiffxp
spiffxp approved these changes Mar 13, 2021
View reviewed changes
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm
Thanks for working on this @supriya-premkumar!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amwat, spiffxp, supriya-premkumar

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

@spiffxp
Copy link
Member

spiffxp commented Mar 13, 2021

Edited to drop the fixes part. We'll need more to plumb this through in kettle and testgrid

@k8s-ci-robot k8s-ci-robot merged commit c560100 into kubernetes:master Mar 13, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Mar 13, 2021
@chewong chewong mentioned this pull request Mar 26, 2021
Merged
@supriya-premkumar supriya-premkumar mentioned this pull request Apr 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spiffxp spiffxp spiffxp approved these changes

@feiskyer feiskyer Awaiting requested review from feiskyer

@MushuEE MushuEE Awaiting requested review from MushuEE

@amwat amwat Awaiting requested review from amwat

Assignees

@dims dims

@spiffxp spiffxp

@BenTheElder BenTheElder

@amwat amwat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/images area/kubetest area/release-eng Issues or PRs related to the Release Engineering subproject 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/release Categorizes an issue or PR as relevant to SIG Release. 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.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.
6 participants
@supriya-premkumar @k8s-ci-robot @BenTheElder @spiffxp @amwat @dims