Skip to content

Deprecate calling kubetest2 via kntest#13329

Merged
knative-prow[bot] merged 16 commits intoknative:mainfrom
borg-land:test-deprecating-kntest
Aug 22, 2023
Merged

Deprecate calling kubetest2 via kntest#13329
knative-prow[bot] merged 16 commits intoknative:mainfrom
borg-land:test-deprecating-kntest

Conversation

@upodroid
Copy link
Member

Testing this change before updating knative/hack

@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Sep 22, 2022
@upodroid
Copy link
Member Author

/test upgrade-tests

@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Patch coverage has no change and project coverage change: +0.01% 🎉

Comparison is base (b3b793f) 86.23% compared to head (83b2195) 86.25%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13329      +/-   ##
==========================================
+ Coverage   86.23%   86.25%   +0.01%     
==========================================
  Files         199      199              
  Lines       14811    14811              
==========================================
+ Hits        12773    12775       +2     
+ Misses       1735     1733       -2     
  Partials      303      303              

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 22, 2022
@upodroid
Copy link
Member Author

@cardil What do you think?

This change allows us to go from kntest kubetest2 gke to kubetest2 gke with the following benefits:

If this looks good, I'll make the changes in knative/hack. This needs to be feature gated for a few releases

@nader-ziada
Copy link
Member

you need to run ./hack/update-codegen.sh. for the verify failure

@upodroid
Copy link
Member Author

I know, I will update this upstream. I wanted to test the change before submitting it upstream as it is very annoying to iterate.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2022
@upodroid
Copy link
Member Author

upodroid commented Nov 9, 2022

/close

I'll pick this up once the signing work is complete.

@knative-prow knative-prow bot closed this Nov 9, 2022
@knative-prow
Copy link

knative-prow bot commented Nov 9, 2022

@upodroid: Closed this PR.

Details

In response to this:

/close

I'll pick this up once the signing work is complete.

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.

@upodroid upodroid reopened this May 7, 2023
@upodroid
Copy link
Member Author

upodroid commented May 7, 2023

I need to actually complete this in May to complete the repo migration. One of the blockers is deprecating kntest

@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2023
if [[ ! " ${_custom_flags[*]} " =~ "--machine-type=" ]]; then
_custom_flags+=("--machine-type=e2-standard-4")
fi
kubetest2 gke "${_custom_flags[@]}" --rundir-in-artifacts --up --down \
Copy link
Member Author

Choose a reason for hiding this comment

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

@dprotaso @evankanderson @cardil @kvmware

Are we happy with this change? I'm going to apply it upstream but every repo will need to make small adjustments similar to the changes I made here to the test/*.sh files. This change works and all the e2e tests are passing. Jobs can pass most kubetest2 args and gcloud args from their specific e2e_tests script.

Tl;dr, we need to deprecate kntest and call kubetest2 directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have an opinion. Just glad we can move on from kntest.
For the test/*.sh changes it would be good to sed replace those and open at same time, or at the very least make sure that is only things that need changing. Would suck to find some repo's test scripts that require a lot of work to get going and are unable to test.

Copy link
Member

Choose a reason for hiding this comment

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

no issues from me

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2023
@upodroid
Copy link
Member Author

I rebased this PR.

@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 18, 2023
@dprotaso
Copy link
Member

/retest

@upodroid
Copy link
Member Author

I'm going to override the lint checks

@upodroid
Copy link
Member Author

/override "style / suggester / shell"

@knative-prow
Copy link

knative-prow bot commented Aug 18, 2023

@upodroid: Overrode contexts on behalf of upodroid: style / suggester / shell

Details

In response to this:

/override "style / suggester / shell"

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.

@krsna-m
Copy link
Contributor

krsna-m commented Aug 21, 2023

/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2023
@dprotaso
Copy link
Member

/lgtm
/approve

@knative-prow
Copy link

knative-prow bot commented Aug 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, upodroid

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

The pull request process is described here

Details 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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2023
@knative-prow knative-prow bot merged commit a9b48be into knative:main Aug 22, 2023
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/test-and-release It flags unit/e2e/conformance/perf test issues for product features lgtm 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.

5 participants