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

PTX-18831 Improving logic for when Telemetry should be enabled/disabled #1166

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

nikolaypopov
Copy link
Collaborator

Improving logic for when Telemetry should be enabled/disabled

Copy link
Contributor

@ezhang-px ezhang-px left a comment

Choose a reason for hiding this comment

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

some of the APIs (GetPxProxyEnvVarValue, CanAccessArcusRegisterEndpoint, ParsePxProxyURL, IsCCMGoSupported) are redefined in other packages: drivers/storage/portworx/component, drivers/storage/portworx/util with even different implementation, can we simply call them for the test purpose instead of rewriting?

pkg/util/test/util.go Show resolved Hide resolved
@nikolaypopov
Copy link
Collaborator Author

some of the APIs (GetPxProxyEnvVarValue, CanAccessArcusRegisterEndpoint, ParsePxProxyURL, IsCCMGoSupported) are redefined in other packages: drivers/storage/portworx/component, drivers/storage/portworx/util with even different implementation, can we simply call them for the test purpose instead of rewriting?

I tried that and it is not possible due to import cycle not allowed, also as per @piyush-nimbalkar we do not want to re-use much of operator functions, just in case a bug is introduced there.

@ezhang-px
Copy link
Contributor

some of the APIs (GetPxProxyEnvVarValue, CanAccessArcusRegisterEndpoint, ParsePxProxyURL, IsCCMGoSupported) are redefined in other packages: drivers/storage/portworx/component, drivers/storage/portworx/util with even different implementation, can we simply call them for the test purpose instead of rewriting?

I tried that and it is not possible due to import cycle not allowed, also as per @piyush-nimbalkar we do not want to re-use much of operator functions, just in case a bug is introduced there.

Got you, thanks for the explanation. @jrivera-px also described more context to me offline about what's going yesterday. Just one catch here about the CanAccessArcusRegisterEndpoint, I think in the current release, it's already updated, so is it on purpose that we have to use the code before the https changes?
https://github.com/libopenstorage/operator/blob/px-rel-23.7.0/drivers/storage/portworx/component/telemetry.go#L1036

Signed-off-by: nikolaypopov <nikolay.popov86@gmail.com>
Copy link
Contributor

@ezhang-px ezhang-px left a comment

Choose a reason for hiding this comment

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

LGTM

@nikolaypopov nikolaypopov merged commit f2a7fbd into master Jul 27, 2023
8 checks passed
@nikolaypopov nikolaypopov deleted the PTX-18831 branch July 27, 2023 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants