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

refactor: rename test/utils -> test/internal #1171

Merged
merged 4 commits into from
Jan 30, 2025
Merged

refactor: rename test/utils -> test/internal #1171

merged 4 commits into from
Jan 30, 2025

Conversation

Tal-or
Copy link
Collaborator

@Tal-or Tal-or commented Jan 28, 2025

Force the convention that test code cannot be used outside of test package.

In addition allow running unit-tests for files under test/internal because there is a test code which is complex and/or being used wide enough that it's worth running unit-tests on it.

@Tal-or Tal-or requested a review from ffromani January 28, 2025 10:06
@openshift-ci openshift-ci bot requested a review from shajmakh January 28, 2025 10:06
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Tal-or

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2025
@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 28, 2025

/hold

depends on #1169

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2025
@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 28, 2025

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2025
swatisehgal
swatisehgal reviewed Jan 28, 2025
View reviewed changes
Copy link
Collaborator

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

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

Some NITs, but overall LGTM

/lgtm

test/internal/README.md Outdated Show resolved Hide resolved
test/internal/README.md Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2025
@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 28, 2025

@swatisehgal Thank you for the review I fixed the typo, please approve again when possible

swatisehgal
swatisehgal reviewed Jan 28, 2025
View reviewed changes
Copy link
Collaborator

@swatisehgal swatisehgal left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2025
ffromani
ffromani reviewed Jan 28, 2025
View reviewed changes
Makefile Outdated Show resolved Hide resolved
@ffromani
Copy link
Member

/hold

PTAL to the inline comments

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2025
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@ffromani
Copy link
Member

/hold cancel
/lgtm

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@ffromani
Copy link
Member

/lgtm cancel

linter lane failures

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 29, 2025

/lgtm cancel

linter lane failures

Fixed.
Not sure what has changed, I don't see the errors when running against the main branch

@ffromani
Copy link
Member

/lgtm cancel
linter lane failures

Fixed. Not sure what has changed, I don't see the errors when running against the main branch

Previously that package was ignored by the linter, we need to update the linter config. That package was copy/pasted from upstream kubernetes, better to keep it 1:1 unchanged. IOW we either update our local clone or fix the linter config to ignore it. Or both.

@Tal-or
Copy link
Collaborator Author

Tal-or commented Jan 29, 2025

/lgtm cancel
linter lane failures

Fixed. Not sure what has changed, I don't see the errors when running against the main branch

Previously that package was ignored by the linter, we need to update the linter config. That package was copy/pasted from upstream kubernetes, better to keep it 1:1 unchanged. IOW we either update our local clone or fix the linter config to ignore it. Or both.

Even the most updated package from k8s contains the same lint error.
I don't have a strong opinion, we may set the linter to ignore, or update the local clone. Whatever works the best

@ffromani
Copy link
Member

/lgtm cancel
linter lane failures

Fixed. Not sure what has changed, I don't see the errors when running against the main branch

Previously that package was ignored by the linter, we need to update the linter config. That package was copy/pasted from upstream kubernetes, better to keep it 1:1 unchanged. IOW we either update our local clone or fix the linter config to ignore it. Or both.

Even the most updated package from k8s contains the same lint error. I don't have a strong opinion, we may set the linter to ignore, or update the local clone. Whatever works the best

let's update the linter config to update that specific part then, and we move on.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2025
Tal-or added 4 commits January 29, 2025 14:13
@Tal-or
tests: rename test/utils -> test/internal
7bf3166 
renaming to emphasis the tools under tests are internal only and
should be used solely by test code.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or
makefile: run unit tests for test/internal
26d096f 
There is a test code which is complex enough and used wide enough
so it worth running unit-tests for it.

Changing the makefile so it will consider unit-tests under `test/internal`
as well.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or
readme: add readme file
7412f86 
Explain the purpose `test/internal` and its alternatives.

Signed-off-by: Talor Itzhak <[email protected]>
@Tal-or
lint: exclude k8simports
9f62b43 
This dir contains helper which were copied verbatim from k/k repo.
we prefer to keep the match 1:1 than fixing the linter issues, so
let exclude this directoy from the lint config.

Signed-off-by: Talor Itzhak <[email protected]>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2025
@ffromani
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2025
@ffromani
Copy link
Member

/retest

@ffromani
Copy link
Member

/test ci-e2e-install-hypershift

@openshift-merge-bot openshift-merge-bot bot merged commit 28becd0 into openshift-kni:main Jan 30, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffromani ffromani ffromani left review comments

@swatisehgal swatisehgal swatisehgal left review comments

@shajmakh shajmakh Awaiting requested review from shajmakh

Assignees

@swatisehgal swatisehgal

@ffromani ffromani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Tal-or @ffromani @swatisehgal @openshift-merge-robot