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

use openshift_context instead of internal function in util package #409

Merged
merged 2 commits into from
Feb 21, 2023
Merged

use openshift_context instead of internal function in util package #409

merged 2 commits into from
Feb 21, 2023

Conversation

SchSeba
Copy link
Collaborator

@SchSeba SchSeba commented Feb 19, 2023

No description provided.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@SchSeba
Copy link
Collaborator Author

SchSeba commented Feb 19, 2023

as requested by @bn222 I create a new PR to always use the ocp context

#365 (comment)

@SchSeba SchSeba mentioned this pull request Feb 19, 2023
Merged
@coveralls
Copy link

coveralls commented Feb 19, 2023 •
edited
Loading

Pull Request Test Coverage Report for Build 4233514086

  • 8 of 37 (21.62%) changed or added relevant lines in 4 files are covered.
  • 26 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.3%) to 24.967%
Changes Missing Coverage Covered Lines Changed/Added Lines %
controllers/sriovnetworkpoolconfig_controller.go 1 3 33.33%
controllers/sriovoperatorconfig_controller.go 3 5 60.0%
pkg/daemon/daemon.go 1 3 33.33%
pkg/utils/openshift_context.go 3 26 11.54%
Files with Coverage Reduction New Missed Lines %
controllers/sriovoperatorconfig_controller.go 1 56.85%
controllers/sriovnetworkpoolconfig_controller.go 2 53.03%
api/v1/helper.go 3 41.2%
controllers/sriovibnetwork_controller.go 4 65.35%
controllers/sriovnetwork_controller.go 4 65.0%
pkg/utils/cluster.go 12 0%
Totals Coverage Status
Change from base Build 4126507459: -0.3%
Covered Lines: 1881
Relevant Lines: 7534
💛 - Coveralls

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

bn222
bn222 reviewed Feb 20, 2023
View reviewed changes
pkg/utils/openshift_context.go Outdated
}

openshiftFlavor := OpenshiftFlavorDefault
if ClusterType == ClusterTypeOpenshift {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe I miss something but if the first conditional in this function checks for ClusterType != ClusterTypeOpenshift, how can this ever be false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are right I change it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see the change

zeeke
zeeke reviewed Feb 20, 2023
View reviewed changes
controllers/sriovoperatorconfig_controller.go Outdated
logger.Error(err, "Fail to get control plane topology")
return err
}
external := r.OpenshiftContext.IsHypershift()
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this can cause panic:

https://go.dev/play/p/hDtR5cPY6Lj?v=goprev

Maybe it's better to change IsHypershift()'s receiver from OpenshiftContext to *OpenshiftContext and add a nil check inside that function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not in favor of passing around pointers just to check for nil. Instead, I think we should pass around the context still, and have it explicitly say it is not Openshift.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch!
I update to first check if we are running on ocp

@SchSeba SchSeba force-pushed the improve_hypershift_check branch from 6508291 to 53f5a6d Compare February 20, 2023 10:42
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@SchSeba SchSeba force-pushed the improve_hypershift_check branch from 53f5a6d to aed354f Compare February 20, 2023 15:41
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@bn222
Copy link
Collaborator

bn222 commented Feb 20, 2023

/lgtm

@github-actions github-actions bot added the lgtm label Feb 20, 2023
zeeke
zeeke reviewed Feb 20, 2023
View reviewed changes
pkg/daemon/daemon.go Outdated Show resolved Hide resolved
@SchSeba SchSeba force-pushed the improve_hypershift_check branch from aed354f to 030ac59 Compare February 21, 2023 14:08
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@SchSeba
use openshift_context instead of internal function in util package
0d682ca 
Signed-off-by: Sebastian Sch <[email protected]>
@SchSeba
small change to make IDE validate to pass
095dc66 
before this change my IDE was complaining that the function is not right

```
Cannot use 'f' (type func()) as the type DialFunc
```

Signed-off-by: Sebastian Sch <[email protected]>
@SchSeba SchSeba force-pushed the improve_hypershift_check branch from 030ac59 to 095dc66 Compare February 21, 2023 14:10
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@bn222
Copy link
Collaborator

bn222 commented Feb 21, 2023

/lgtm

@bn222 bn222 merged commit cb98769 into k8snetworkplumbingwg:master Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bn222 bn222 bn222 left review comments

@zeeke zeeke zeeke left review comments

@adrianchiris adrianchiris adrianchiris approved these changes

Assignees
No one assigned
Labels
lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SchSeba @coveralls @bn222 @zeeke @adrianchiris