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

fix: import package with same prefix name as the root package #2598

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

leovct
Copy link
Collaborator

@leovct leovct commented Nov 29, 2024

Description

An attempt to fix the issue when importing a package with the same prefix name as the root package (e.g. importing kurtosis-test-xyz inside kurtosis-test)

I added two new test cases to show it's not working properly:

func TestGetAbsoluteLocator_RepositoriesWithSamePrefixNameShouldNotBeBlocked(t *testing.T) {
	provider := NewGitPackageContentProvider("", "", NewGitHubPackageAuthProvider(""), nil)

	packageId := "github.com/main-package"
	locatorOfModuleInWhichThisBuiltInIsBeingCalled := "github.com/main-package-xyz/main.star" // same package prefix
	maybeRelativeLocator := "github.com/main-package/file.star"

	_, err := provider.GetAbsoluteLocator(packageId, locatorOfModuleInWhichThisBuiltInIsBeingCalled, maybeRelativeLocator, noPackageReplaceOptions)
	require.Nil(t, err)
}

func Test_isNotSamePackageLocalAbsoluteLocator_TestRepositoriesWithSamePrefixNames(t *testing.T) {
	result := shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage("github.com/author/package2/main.star", "github.com/author/package/main.star", "github.com/author/package")
	require.False(t, result)
}
--- FAIL: TestGetAbsoluteLocator_RepositoriesWithSamePrefixNameShouldNotBeBlocked (0.00s)
    git_package_content_provider_test.go:611: 
                Error Trace:    /home/circleci/project/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider_test.go:611
                Error:          Expected nil, but got: &startosis_errors.InterpretationError{msg:"Locator 'github.com/main-package/file.star' is referencing a file within the same package using absolute import syntax, but only relative import syntax (path starting with '/' or '.') is allowed for within-package imports", cause:error(nil), stacktrace:[]startosis_errors.CallFrame(nil)}
                Test:           TestGetAbsoluteLocator_RepositoriesWithSamePrefixNameShouldNotBeBlocked
FAIL
FAIL    github.com/kurtosis-tech/kurtosis/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider     5.120s

--- FAIL: Test_isNotSamePackageLocalAbsoluteLocator_TestRepositoriesWithSamePrefixNames (0.00s)
    git_package_content_provider_test.go:626: 
                Error Trace:    /home/circleci/project/core/server/api_container/server/startosis_engine/startosis_packages/git_package_content_provider/git_package_content_provider_test.go:626
                Error:          Should be false
                Test:           Test_isNotSamePackageLocalAbsoluteLocator_TestRepositoriesWithSamePrefixNames

The problem comes from the fact that the function only checks if the locator contains the package id (prefix check) instead of carrying out a more rigorous check:

func shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage(relativeOrAbsoluteLocator string, sourceModuleLocator string, rootPackageId string) bool {
	isSourceModuleInRootPackage := strings.HasPrefix(sourceModuleLocator, rootPackageId)
	isAbsoluteLocatorInRootPackage := strings.HasPrefix(relativeOrAbsoluteLocator, rootPackageId)
	return isSourceModuleInRootPackage && isAbsoluteLocatorInRootPackage
}

To fix this I made sure the rootPackageId ends with a trailing slash. This way it should not complain when checking absolute locators with the same prefix.

func shouldBlockAbsoluteLocatorBecauseIsInTheSameSourceModuleLocatorPackage(relativeOrAbsoluteLocator string, sourceModuleLocator string, rootPackageId string) bool {
	// Make sure the root package id ends with a trailing slash.
	rootPackageId = strings.TrimSuffix(rootPackageId, "/") + "/"
	isSourceModuleInRootPackage := strings.HasPrefix(sourceModuleLocator, rootPackageId)
	isAbsoluteLocatorInRootPackage := strings.HasPrefix(relativeOrAbsoluteLocator, rootPackageId)
	return isSourceModuleInRootPackage && isAbsoluteLocatorInRootPackage
}

REMINDER: Tag Reviewers, so they get notified to review

Is this change user facing?

YES

References (if applicable)

@leovct leovct force-pushed the fix/importing-packatge-with-same-prefix-name branch from e8ac6b0 to 623fbfb Compare November 29, 2024 11:51
@leovct leovct changed the title test: import package with same prefix name fix: import package with same prefix name Nov 29, 2024
@leovct leovct marked this pull request as ready for review November 29, 2024 12:35
@leovct leovct changed the title fix: import package with same prefix name fix: import package with same prefix name as the root package Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant