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

Populate package-path metadata into package context #3596

Merged
merged 1 commit into from
Oct 15, 2022
Merged

Populate package-path metadata into package context #3596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 29 additions & 10 deletions internal/builtins/pkg_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ import (

const (
PkgContextFile = "package-context.yaml"
pkgContextName = "kptfile.kpt.dev"
PkgContextName = "kptfile.kpt.dev"

ConfigKeyPackagePath = "package-path"
)

var (
Expand All @@ -42,7 +44,17 @@ var (
// a KRM object that contains package context information that can be
// used by functions such as `set-namespace` to customize package with
// minimal configuration.
type PackageContextGenerator struct{}
type PackageContextGenerator struct {
// PackageConfig contains the package configuration to set.
PackageConfig *PackageConfig
}

// PackageConfig holds package automatic configuration
type PackageConfig struct {
// PackagePath is the path to the package, as determined by the names of the parent packages.
// The path to a package is the parent package path joined with the package name.
PackagePath string
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share what this is going to look like in CLI and porch based workflow. Is it relative ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So as I have it now, it's going to look like a directory path, myorg/apps/coolapp/dev or something. I'm not certain what comes at the start of that (for example, should we try to make these globally unique, or at least allow that, vs being relative to the repository?) I'm also not certain whether we should include some notion of the package type, in which case the package path would end up looking like a GCP URL organizations/myorg/folders/apps/applications/coolapp/environments/dev.

I figure we can iterate a bit here.

I think it would be the same in the CLI, though I'm not yet sure what we should do if you move it (which is the same problem we have if you rename the package - should we update the name in package-config.yaml?)

Copy link
Contributor

Choose a reason for hiding this comment

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

for example, should we try to make these globally unique, or at least allow that, vs being relative to the repository?

relative to the repository in a deployment repo is the immediate usecase that I can relate with. Others are less clear to me as well. [Since we don't operate in context of repo in pure CLI experience, we can probably keep this no-op in CLI mode.]

About including package-type in the package paths feels like mixing logical view of your packages with the filesystem view (if they map 1:1 well, then we are good, otherwise screwed) :)

I think refactoring a package (like move package to new location or rename), I think the only way to handle them properly, will be to introduce refactoring functionality in our workflows for ex. pkg rename|move|.... if done through our tooling, we can update the underlying metadata automatically.

}

// Run function reads the function input `resourceList` from a given reader `r`
// and writes the function output to the provided writer `w`.
Expand All @@ -66,14 +78,14 @@ func (pc *PackageContextGenerator) Process(resourceList *framework.ResourceList)
// - Generates a package context resource for each kpt package (i.e Kptfile)
for _, resource := range resourceList.Items {
gvk := resid.GvkFromNode(resource)
if gvk.Equals(configMapGVK) && resource.GetName() == pkgContextName {
if gvk.Equals(configMapGVK) && resource.GetName() == PkgContextName {
// drop existing package context resources
continue
}
updatedResources = append(updatedResources, resource)
if gvk.Equals(kptfileGVK) {
// it's a Kptfile, generate a corresponding package context
pkgContext, err := pkgContextResource(resource)
pkgContext, err := pkgContextResource(resource, pc.PackageConfig)
if err != nil {
resourceList.Results = framework.Results{
&framework.Result{
Expand Down Expand Up @@ -102,10 +114,10 @@ func (pc *PackageContextGenerator) Process(resourceList *framework.ResourceList)

// pkgContextResource generates package context resource from a given
// Kptfile. The resource is generated adjacent to the Kptfile of the package.
func pkgContextResource(kf *yaml.RNode) (*yaml.RNode, error) {
func pkgContextResource(kptfile *yaml.RNode, packageConfig *PackageConfig) (*yaml.RNode, error) {
cm := yaml.MustParse(AbstractPkgContext())

kptfilePath, _, err := kioutil.GetFileAnnotations(kf)
kptfilePath, _, err := kioutil.GetFileAnnotations(kptfile)
if err != nil {
return nil, err
}
Expand All @@ -118,9 +130,16 @@ func pkgContextResource(kf *yaml.RNode) (*yaml.RNode, error) {
return nil, err
}
}
cm.SetDataMap(map[string]string{
"name": kf.GetName(),
})
data := map[string]string{
"name": kptfile.GetName(),
}
if packageConfig != nil {
if packageConfig.PackagePath != "" {
data[ConfigKeyPackagePath] = packageConfig.PackagePath
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing this conditional will ensure package-path is not added for kpt CLI based workflows ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe so, correct. It'll set it only if we have something to set.

Based on the discussion above, I sort of feel like we shouldn't be persisting either package-path or package-name into the package-context.yaml. Rather we should be generating it on the fly every time; then we don't have a conflict between directory name / directory path vs package-name/package-path.

The exception to that would be if we wanted the original name, so that if we generated something immutable it would be preserved. However, we do have other answers there, like trying to capture the name in the parameters in the kpt function pipeline or similar.

Sounds like we need to think more about the challenges here, but that we should merge this so we can gain some experience with how it behaves in practice.


cm.SetDataMap(data)
return cm, nil
}

Expand All @@ -136,5 +155,5 @@ metadata:
config.kubernetes.io/local-config: "true"
data:
name: example
`, pkgContextName)
`, PkgContextName)
}
12 changes: 4 additions & 8 deletions porch/pkg/engine/builtin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,13 @@ type builtinEvalMutation struct {
runner fn.FunctionRunner
}

func newBuiltinFunctionMutation(function string) (mutation, error) {
var runner fn.FunctionRunner
switch function {
case fnruntime.FuncGenPkgContext:
runner = &builtins.PackageContextGenerator{}
default:
return nil, fmt.Errorf("unrecognized built-in function %q", function)
func newPackageContextGeneratorMutation(packageConfig *builtins.PackageConfig) (mutation, error) {
runner := &builtins.PackageContextGenerator{
PackageConfig: packageConfig,
Copy link
Contributor

Choose a reason for hiding this comment

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

I see it now. This confirms this will be done only in porch based workflow.

}

return &builtinEvalMutation{
function: function,
function: fnruntime.FuncGenPkgContext,
runner: runner,
}, nil
}
Expand Down
15 changes: 5 additions & 10 deletions porch/pkg/engine/builtin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,14 @@ import (
"path/filepath"
"testing"

"github.com/GoogleContainerTools/kpt/internal/fnruntime"
"github.com/GoogleContainerTools/kpt/internal/builtins"
"github.com/google/go-cmp/cmp"
)

const (
updateGoldenFiles = "UPDATE_GOLDEN_FILES"
)

func TestUnknownBuiltinFunctionMutation(t *testing.T) {
const doesNotExist = "function-does-not-exist"
m, err := newBuiltinFunctionMutation(doesNotExist)
if err == nil || m != nil {
t.Errorf("creating builtin function runner for an unknown function %q unexpectedly succeeded", doesNotExist)
}
}

func TestPackageContext(t *testing.T) {
testdata, err := filepath.Abs(filepath.Join(".", "testdata", "context"))
if err != nil {
Expand All @@ -44,7 +36,10 @@ func TestPackageContext(t *testing.T) {

input := readPackage(t, filepath.Join(testdata, "input"))

m, err := newBuiltinFunctionMutation(fnruntime.FuncGenPkgContext)
packageConfig := &builtins.PackageConfig{
PackagePath: "parent1/parent1.2/parent1.2.3/me",
}
m, err := newPackageContextGeneratorMutation(packageConfig)
if err != nil {
t.Fatalf("Failed to get builtin function mutation: %v", err)
}
Expand Down
9 changes: 6 additions & 3 deletions porch/pkg/engine/clone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"os"
"strings"

"github.com/GoogleContainerTools/kpt/internal/fnruntime"
"github.com/GoogleContainerTools/kpt/internal/builtins"
v1 "github.com/GoogleContainerTools/kpt/pkg/api/kptfile/v1"
api "github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
configapi "github.com/GoogleContainerTools/kpt/porch/api/porchconfig/v1alpha1"
Expand All @@ -44,6 +44,9 @@ type clonePackageMutation struct {
repoOpener RepositoryOpener
credentialResolver repository.CredentialResolver
referenceResolver ReferenceResolver

// packageConfig contains the package configuration.
packageConfig *builtins.PackageConfig
}

func (m *clonePackageMutation) Apply(ctx context.Context, resources repository.PackageResources) (repository.PackageResources, *api.Task, error) {
Expand Down Expand Up @@ -77,13 +80,13 @@ func (m *clonePackageMutation) Apply(ctx context.Context, resources repository.P
if m.isDeployment {
// TODO(droot): executing this as mutation is not really needed, but can be
// refactored once we finalize the task/mutation/commit model.
genPkgContextMutation, err := newBuiltinFunctionMutation(fnruntime.FuncGenPkgContext)
genPkgContextMutation, err := newPackageContextGeneratorMutation(m.packageConfig)
if err != nil {
return repository.PackageResources{}, nil, err
}
cloned, _, err = genPkgContextMutation.Apply(ctx, cloned)
if err != nil {
return repository.PackageResources{}, nil, fmt.Errorf("failed to generate deployment context %w", err)
return repository.PackageResources{}, nil, fmt.Errorf("failed to generate deployment context: %w", err)
}
}

Expand Down
Loading