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

Remove symlink to crd file in kubernetes e2e test data #10388

Merged
merged 8 commits into from
Nov 22, 2024

Conversation

sheidkamp
Copy link

@sheidkamp sheidkamp commented Nov 21, 2024

Description

A symlink in the test data causes test failure/file-not-found when imported into solo-projects

Code changes

Added Directory() to the projects/gateway2/crds package to provide a way to access the crd files.

Removed symlink and updated types.go to point to the original file using crds.Directory()

Testing steps

Locally, test still passes:

ISTIO_VERSION=1.21.0  GOLANG_PROTOBUF_REGISTRATION_CONFLICT=ignore go test -v -timeout 600s ./test/kubernetes/e2e/tests -run ^TestK8sGateway$/^HTTPRouteServices$/^TestConfigureHTTPRouteBackingDestinationsWithServiceAndWithoutTCPRoute$
...
  Gloo was successfully uninstalled.
--- PASS: TestK8sGateway (113.27s)
    --- PASS: TestK8sGateway/HTTPRouteServices (46.08s)
        --- PASS: TestK8sGateway/HTTPRouteServices/TestConfigureHTTPRouteBackingDestinationsWithServiceAndWithoutTCPRoute (46.08s)

Validated by pulling into solo-projects:

go get github.com/solo-io/gloo@remove-manifest-symlink

and running tests on an existing cluster:

go clean -testcache; GOLANG_PROTOBUF_REGISTRATION_CONFLICT=ignore go test -v -timeout 600s ./test/kubernetes/e2e/tests -run  ^TestOssFeatures$/^OssK8sGateway$/^HTTPRouteServices$/^TestConfigureHTTPRouteBackingDestinationsWithServiceAndWithoutTCPRoute$

With output:

....
  Gloo was successfully uninstalled.
--- PASS: TestOssFeatures (100.21s)
    --- PASS: TestOssFeatures/OssK8sGateway (100.21s)
        --- PASS: TestOssFeatures/OssK8sGateway/HTTPRouteServices (48.20s)
            --- PASS: TestOssFeatures/OssK8sGateway/HTTPRouteServices/TestConfigureHTTPRouteBackingDestinationsWithServiceAndWithoutTCPRoute (48.20s)
PASS

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@solo-changelog-bot
Copy link

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/7255

@@ -15,9 +15,12 @@ import (
)

var (
// This is fragile, but symlinks don't work when the tests are imported to another repo, and its better than duplicating the file
repoRoot = filepath.Join(util.MustGetThisDir(), "..", "..", "..", "..", "..", "..")

Choose a reason for hiding this comment

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

settingsFixturesFolder := filepath.Join(util.GetModuleRoot(), "install", "test", "fixtures", "settings")

Can we use GetModuleRoot instead?

Alternatively, we could define a method in the crds/ package that returns the full path to a file, and then call it here. that way the functionalty lives in the package itself and others can use

Copy link
Author

Choose a reason for hiding this comment

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

That does not work when imported into solo-projects

Copy link
Author

Choose a reason for hiding this comment

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

hmm, and what I tried was GetGoModule not GetModuleRoot, so let me try that too

Choose a reason for hiding this comment

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

If we exposed a method in the crds package that usesd "GetThisDir/file-name", couldn't that work?

Copy link
Author

Choose a reason for hiding this comment

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

That does work

@sheidkamp sheidkamp enabled auto-merge (squash) November 22, 2024 17:07
@sheidkamp sheidkamp merged commit 25aa232 into main Nov 22, 2024
17 of 18 checks passed
@sheidkamp sheidkamp deleted the remove-manifest-symlink branch November 22, 2024 17:40
ryanrolds pushed a commit that referenced this pull request Dec 10, 2024
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.

3 participants