internal/profile: export profile loading function

[efd6]

For #2868

Please take a look.

[jsoriano]

Is this going to be used out of the package?

[efd6]

Yes

[jsoriano]

Could you give more deatils about how this is going to be used out of the package?

[jsoriano]

At the moment migration is part of the profile loading process, it shouldn't be needed out of the package. Do you have a use case for this?

If this is intended to load from a directory with the migrations included, maybe we could keep loadProfile private, and create a method LoadProfileFrom, that does the same as LoadProfile, including the migrations. So callers don't need to care about migrations.

Something like this:

func LoadProfile(profileName string) (*Profile, error) {
	loc, err := locations.NewLocationManager()
	if err != nil {
		return nil, fmt.Errorf("error finding stack dir location: %w", err)
	}

	return LoadProfileFrom(profileName, loc.ProfileDir())
}

func LoadProfileFrom(profileName, profilesDir string) (*Profile, error) {
	profile, err := loadProfile(profilesDir, profileName)
	if err != nil {
		return nil, err
	}

	err = profile.migrate(currentVersion)
	if err != nil {
		return nil, fmt.Errorf("error migrating profile to version %v: %w", currentVersion, err)
	}

	return profile, nil
}

[efd6]

Given the choice of reduced code duplication/increased code complixity, and reduced exported functions in an internal package, I chose the latter.

[jsoriano]

It is not so much about functions exported or code duplication, but about responsibilities.
As the package is defined now the responsibility of migrating profiles clearly belongs to the profile package.
If we expose different methods for load and migration of profiles, this responsibility is diluted.
Also, exposing two different "LoadProfile" methods that do different things is prone to errors (LoadProfile would run migrations, but LoadProfileFrom wouldn't).

[efd6]

The changes that are in this are the minimal set that I need to be able to implement a testscript hook for bringing up/taking down a stack.

[jsoriano]

The changes that are in this are the minimal set that I need to be able to implement a testscript hook for bringing up/taking down a stack.

I understand how loading specific profiles may be needed, but running migrations from different places looks error-prone. Why is this needed?

[efd6]

I don't specifically need the migration method, but this is the minimal change to get the behaviour that is needed given the way that testing is done here. Ideally, I'd not do it, but the proposal above it more complicated for IMO limited benefit.

[jsoriano]

I don't specifically need the migration method, but this is the minimal change to get the behaviour that is needed given the way that testing is done here. Ideally, I'd not do it, but the proposal above it more complicated for IMO limited benefit.

I think I am missing what is the needed behavior. Other packages can already load profiles with LoadProfile. They only cannot choose the directory where profiles are stored. If this is needed, in my opinion just adding a LoadProfileFrom method as proposed in #2916 (comment) introduces much less complexity than having to use the migration functionality out of the profile package.

[efd6]

If this is needed, in my opinion just adding a LoadProfileFrom method as proposed in #2916 (comment) introduces much less complexity than having to use the migration functionality out of the profile package.

I this is insisted, I will do this, but I disagree.

Specifically, "having to use the migration functionality out of the profile package" is not part of the required behaviour, though I can see test situations where it may be, and in that case, it would then have to be exported. I do not see the benefit from hiding functionality in a package that is internal, and I am seeing significant composability friction because of the level of non-exported labels in these packages.

[jsoriano]

I this is insisted, I will do this, but I disagree.

I am sorry for the disagreement, but I would greatly prefer to avoid exposing migration functionality if not needed, yes. Also, if we expose two methods for loading profiles, I would prefer both to be consistent regarding migrations (or any other thing that they do).

I can see test situations where it may be, and in that case, it would then have to be exported.

One of the advantages of keeping separated responsibilities is that other packages shouldn't need to care about this functionality, including its testing.

If we happen to find a situation where we would benefit from exposing this functionality out of this package I would be happy to do it. Till then I would prefer to keep it separated.

[elasticmachine]

⏳ Build in-progress, with failures

• Buildkite Build
• Commit: 6325ab8

Failed CI Steps

• :linux: Run unit tests
• :macos: Run unit tests

History

• 💚 Build #6137 succeeded be5209a
• 💚 Build #6122 succeeded 0e30fc2

cc @efd6

[mrodm]

LGTM!