Hierarchical and structured config implementation: the initial pass #10832

Zaid-Ajaj · 2022-09-22T16:10:14Z

Description

This implements the initial pass of hierarchical and structured config which fixes #10602.

This changes the CLI such that configuration can now be defined at the project level using a config block. The configuration values defined here are inherited by all the stacks and made available to the Pulumi program without having to duplicate values in every stack (hence hierarchical) and the values are also typed / structured.

Example Project.yaml syntax:

name: config-test
runtime: dotnet
config:
  instanceSize:
    type: string
    default: t3.micro
  instanceCount: 
    type: integer
    default: 5

This can also be rewritten using short-hand syntax and will be equivalent to the above

name: config-test
runtime: dotnet
config:
  instanceSize: t3.micro
  instanceCount: 5

The complex types allowed for now are only arrays and nested arrays:

name: config-test
runtime: dotnet
config:
  availabilityZones:
    type: array
    items: 
      type: string
    default: [us-east-1-atl-1a, us-east-1-chi-1a]

Project-level configuration values that do not have a default value MUST be defined at the stack level
Stack configuration values are type-checked against their defined type in the project file i.e. Pulumi.yaml
Short-hand syntax only accepts primitive values (no arrays for now)
Accepted config types are a subset of a JSON schema where the property type: string | integer | boolean | array is expected. When type: array then a config block must also have property items which defines the type of array elements (can be nested)
Running pulumi config will list the configuration values from the selected stack AND the values inherited from the project
After a successful pulumi up run using hierarchical config from the project, pulumi config refresh will write ALL the used config back to the refreshed stack
pulumi config set/rm only applies to the selected stack

Checklist

I have added tests that prove my fix is effective or that my feature works

I have run make changelog and committed the changelog/pending/<file> documenting my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

pulumi-bot · 2022-09-22T16:11:07Z

Changelog

[uncommitted] (2022-10-17)

Features

[cli] Implement initial MVP for hierarchical and structured project configuration.
#10832
#10832

Frassle · 2022-09-22T17:42:13Z

changelog/pending/20220922--cli-initial-mvp-config.yaml

+changes:
+- type: feat
+  scope: cli
+  description: Implement initial MVP for hierarchical and structured project configuration


Suggested change

description: Implement initial MVP for hierarchical and structured project configuration

description: Implement initial MVP for hierarchical and structured project configuration.

Frassle · 2022-09-22T17:53:56Z

pkg/cmd/pulumi/import.go

@@ -492,7 +492,11 @@ func newImportCmd() *cobra.Command {
 				return result.FromError(fmt.Errorf("getting secrets manager: %w", err))
 			}

-			cfg, err := getStackConfiguration(ctx, s, sm)
+			cfg, err := getStackConfiguration(ctx, s, sm, StackConfigOptions{
+				// we don't need project config here


Really? Maybe for this first mvp, but as soon as we allow things like "aws:region" at project level even import is going to need it. I'd think it simpler to just plumb projects through everywhere and not bother introducing this StackConfigOptions type which we'll probably remove later.

I wasn't sure in this case, so I changed this to inherit project-level config too ✅

pkg/cmd/pulumi/config.go

sdk/go/common/resource/config/value.go

sdk/go/common/workspace/project.go

Frassle · 2022-09-23T08:59:37Z

sdk/go/common/workspace/project.go

 		return errors.New("project is missing a 'runtime' attribute")
 	}

+	project = RewriteShorthandConfigValues(project)


Really the JSON schema ought to be written such that the short-hand config values are validated ok. People could use that schema directly in vscode for example. So I feel this rewrite ought to happen after schema validation.

The project schema now validates short-hand config syntax ✅

Great but the rewrite here should still happen after the Validate call below.
Validate the users input as is, then rewrite to the internal domain model.

Frassle · 2022-09-23T09:02:07Z

sdk/go/common/workspace/paths.go

 	}

 	fileName := fmt.Sprintf("%s.%s%s", ProjectFile, qnameFileName(stackName), filepath.Ext(projPath))

-	// Back compat: StackConfigDir used to be called Config.


We still need to support this. People could still have projects using "config: some_path". Ripping this out is another 4.0 change (maybe we should start leaving "TODO(4.0)" comments around for stuff like this)

Brought back support for defining config: some_path by rewriting it to stackConfigDir when it is a string. Erroring out when both are defined ✅

Frassle · 2022-09-23T09:03:49Z

sdk/go/common/workspace/project.json

+      },
+      "config": {
+        "description": "A map of configuration keys to their types",
+        "type": "object",


Suggested change

"type": "object",

"type": ["object", "string", "null"]

Still need to support the old string path here, and it's optional so a null is also valid.

Frassle · 2022-09-23T09:04:30Z

sdk/go/common/workspace/project.json

+        ]
+      },
+      "config": {
+        "description": "A map of configuration keys to their types",


Add a bit to the description about how the "Config directory location relative to the location of Pulumi.yaml." is a deprecated use of this key.

Added that to the description

Frassle · 2022-09-23T09:10:18Z

sdk/go/common/workspace/project.json

+              "$ref": "#/$defs/configItemsType"
+            }
+          },
+          "if": {


I wonder if a "oneOf" results in better errors here than "if" 🤔
Not needed for this first pass but I might have a play with that idea later, so which is nicer UX.

…g input project

Zaid-Ajaj · 2022-09-24T13:00:56Z

Can't loadProjectStack do this?

The reason loadProjectStack doesn't already apply inherited configuration values is because we do not always want to populate the configuration: especially for pulumi config set and pulumi config rm because when we save the config to disk, it will have the configuration from the project written out too and we don't want that.

We could remove the project *Project parameter but I think it is needed to remove the requirement of name-spacing configuration values with the project name: i.e. project:config-key is now required but I think it shouldn't be. Only config-key is needed and the project name should be auto detected

… files

…tion (less rigid contains check)

Zaid-Ajaj · 2022-10-04T19:25:15Z

I still think loadProjectStack should be handling this

@Frassle As discussed, because of projectStack.Save() we cannot always load the config into the project stack so we only apply where needed.

I think we can just get rid of StackConfigOptions, I don't forsee any options here and applyProjectConfig should always be true for now.

@Frassle Done ✅

Would it be possible to collect these [missing keys error]? It would be a much better UX if we showed users all of the missing keys, instead of just the first one encountered.

@iwahbe Nice catch! Done ✅

I've addressed your comments in the latest update and added moaar tests, please have a second look when you have time 🙏 @Frassle @iwahbe

…ePulumiRepo

iwahbe · 2022-10-05T00:20:27Z

sdk/go/common/workspace/project.go

+type ProjectConfigItemsType struct {
+	Type  string                  `json:"type" yaml:"type"`
+	Items *ProjectConfigItemsType `json:"items" yaml:"items"`
+}
+
+type ProjectConfigType struct {
+	Type        string                  `json:"type" yaml:"type"`
+	Description string                  `json:"description" yaml:"description"`
+	Items       *ProjectConfigItemsType `json:"items" yaml:"items"`
+	Default     interface{}             `json:"default" yaml:"default"`
+}
+


We probably want some omitemptys here.

sdk/go/common/workspace/project.go

iwahbe · 2022-10-05T00:29:07Z

sdk/go/common/workspace/project.go

+type ProjectConfigItemsType struct {
+	Type  string                  `json:"type" yaml:"type"`
+	Items *ProjectConfigItemsType `json:"items" yaml:"items"`
+}


Would it be possible to express this as a schema.Type? We already have a lot of logic to work on these, and we will need to translate them into schema types anyways for entry into the Pulumi YAML type checker.

It's possible to write a function to translate a config type into schema.Type but I wouldn't want to start with this right off the bat and only support what is needed because it would seem that a config type can be any schema.Type but that is not the case

Agree with Zaid here, config types != schema types, it will probably end up being a subset but possibly a non-intersecting set.

We will definitely need the config type -> schema type function when we incorporate this change into Pulumi YAML. We can wait for this change to settle before we unify.

That's a good point that this will probably have to be a subset otherwise there would be config values that YAML wouldn't be able to use, but I think given the subset nature of it I'm going to continue err'ing towards us not making it a schema.Type for now but it's own thing.

Actually see above, I think it might end up being non-intersecting still if we end up with config types that YAML doesn't support.

iwahbe · 2022-10-05T00:30:07Z

sdk/go/common/workspace/project.go

+	return typeName
+}
+
+func ValidateConfigValue(typeName string, itemsType *ProjectConfigItemsType, value interface{}) bool {


I would appreciate a doc comment explaining what constitutes a valid config value.

…ks that pulumi config goes through project config

…rrectly

Frassle · 2022-10-06T18:01:28Z

pkg/backend/filestate/crypto.go

+	if err != nil {
+		return nil, err
+	}
+
 	if configFile == "" {


Right I've cleaned up configFile now on master, so it should always set here.

Frassle · 2022-10-06T18:01:41Z

pkg/backend/httpstate/crypto.go

@@ -25,7 +25,16 @@ import (
 func NewServiceSecretsManager(s Stack, stackName tokens.Name, configFile string) (secrets.Manager, error) {
 	contract.Assertf(stackName != "", "stackName %s", "!= \"\"")

-	info, err := workspace.LoadProjectStack(configFile)
+	project, path, err := workspace.DetectProjectStackPath(stackName.Q())


As above, configFile should always be set here.

Frassle · 2022-10-07T12:48:36Z

tests/integration/integration_test.go

@@ -986,7 +986,6 @@ func TestDestroyStackRef(t *testing.T) {

 	e.RunCommand("pulumi", "up", "--skip-preview", "--yes")

-	e.CWD = os.TempDir()


Oh this was by design so that destroy didn't need a Pulumi.yaml file. We should see if we can maintain that support for now.

…utside

Zaid-Ajaj · 2022-10-12T14:59:13Z

@Frassle I've fixed the issue about pulumi destroy requiring a project file, that is no longer the case and TestDestroyStackRef works nice as it did before. Please have another look when you have time 🙏

sdk/go/common/resource/config/value.go

Frassle · 2022-10-13T11:17:16Z

sdk/go/common/workspace/project.json

@@ -1,180 +1,307 @@
 {
-    "$schema": "https://json-schema.org/draft/2020-12/schema",


The indent change in this file makes diff compare very hard :( can we revert to 4 space tab?

sdk/go/common/workspace/paths_test.go

sdk/go/common/workspace/project.go

Frassle · 2022-10-13T11:21:28Z

sdk/go/common/workspace/project.go

 		return errors.New("project is missing a 'runtime' attribute")
 	}

+	project = RewriteShorthandConfigValues(project)


Great but the rewrite here should still happen after the Validate call below.
Validate the users input as is, then rewrite to the internal domain model.

…r test to assert full stackConfigDir error message

… is it: allow config to be string for validation

changelog/pending/20220922--cli-initial-mvp-config.yaml

Frassle · 2022-10-17T08:39:05Z

sdk/go/common/workspace/loaders.go

+		return nil, err
+	}
+
+	if projectStackRaw == nil {


I am very suspect of this. I assume this was to fix the "destory works without a Pulumi.yaml" but fixing it at this level means that any command using projects will try to use an empty project rather than failing validation, that feels wrong.

This was not the fix for "destory works without a Pulumi.yaml", the fix was in removing readProject() from getStackConfiguration and refactor getStackConfiguration to accept a project from the outer scope (which in case of destroy, accounts for non-existing Pulumi.yaml)

This however, I believe is a check for when there is a stack file Pulumi.<stack>.yaml but it is empty so the unmarshalled projectStackRaw becomes nil. In this case, I treat it the same way we do when the file does not exist by returning a default project stack

Frassle · 2022-10-17T08:40:13Z

sdk/go/common/workspace/loaders.go

@@ -105,8 +106,27 @@ func (singleton *projectLoader) load(path string) (*Project, error) {
 		return nil, fmt.Errorf("could not validate '%s': %w", path, err)
 	}

+	// just before marshalling, we will rewrite the config values


I still think this rewrite should happen after validation.

I removed the rewriting from inside the validation code but didn't realize it was duplicated in the loader code. I will adjust it

Zaid-Ajaj · 2022-10-18T16:42:01Z

bors merge

bors · 2022-10-18T17:11:00Z

Build succeeded:

bors-ok

Zaid-Ajaj added 9 commits September 20, 2022 20:13

Initial work on project-level config (MVP)

6c6b5ce

Implement stack config validation and inheritance

683f7e1

Add description to the config type declaration

f659376

Apply project config to stack config when appropriate

966e96f

Implement short-hand configuration value support

aa648fd

support string, int and bool in shorthand config definitions

dff4079

test config schemas that use short hand version

6531b66

Moaar unit tests for config validation

d419605

changelog entry

02bdbd0

Zaid-Ajaj added area/cli UX of using the CLI (args, output, logs) impact/no-changelog-required This issue doesn't require a CHANGELOG update labels Sep 22, 2022

Zaid-Ajaj requested a review from a team September 22, 2022 16:10

lint

7975498

Frassle removed the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Sep 22, 2022

Frassle reviewed Sep 23, 2022

View reviewed changes

Zaid-Ajaj added 5 commits September 24, 2022 13:50

Merge branch 'master' into minimal-config

78ecb2b

project.json should validate short-hand config syntax before rewritin…

969e0e6

…g input project

Rewrite config to stackConfigDir when provided as string

0498be1

pulumi import uses hierarchical config

65cfee4

namespaced config values don't need the project as root namespace

5bd9b80

Zaid-Ajaj added 5 commits September 26, 2022 12:30

Project namespace is now optional when defining stack config

fbc68cb

Fix reading empty project stack file

77c7880

Fix TestSecretsProviderOverride which requires both stack and project…

ae03899

… files

lint

46be886

Fix TestDestroyStackRef: don't change CWD and fix TestStackInitValida…

2755638

…tion (less rigid contains check)

Zaid-Ajaj mentioned this pull request Sep 27, 2022

Cannot up dynamically created stacks using 3.40.2 #10862

Closed

simplify test assert even more (attempt #3)

afeac6e

Zaid-Ajaj requested a review from Frassle September 27, 2022 13:53

Fix tests that have incomplete project config. Correct docs for Creat…

c739adb

…ePulumiRepo

iwahbe reviewed Oct 5, 2022

View reviewed changes

Zaid-Ajaj added 4 commits October 5, 2022 15:10

Use "omit empty" for config types, add another integration test, chec…

4cb2c19

…ks that pulumi config goes through project config

Merge branch 'master' into minimal-config

37730d4

Merge branch 'master' into minimal-config

00f4ba3

Add array config to test that the array is marshalled/unmarshalled co…

813ac76

…rrectly

Frassle reviewed Oct 6, 2022

View reviewed changes

Assume configFile is non-empty

7c509d2

Frassle reviewed Oct 7, 2022

View reviewed changes

Zaid-Ajaj added 5 commits October 12, 2022 15:38

Merge branch 'master' into minimal-config

0766b86

Revert changing the temp dir TestDestroyStackRef

12f3992

fix TestProjectLoadYAML expected error messages

c606757

Allow project to be nil when namespacing project stack config

42a51fe

remove readProject from getStackConfiguration and pass project from o…

51b1329

…utside

Frassle reviewed Oct 13, 2022

View reviewed changes

No need to rewrite project when validating project schema and refacto…

fb11347

…r test to assert full stackConfigDir error message

Zaid-Ajaj requested a review from Frassle October 13, 2022 17:53

Don't know why the change in the project schema was reverted but here…

58de26d

… is it: allow config to be string for validation

Frassle reviewed Oct 17, 2022

View reviewed changes

Update changelog/pending/20220922--cli-initial-mvp-config.yaml

f2ac21f

Frassle approved these changes Oct 17, 2022

View reviewed changes

Frassle mentioned this pull request Oct 17, 2022

Update pulumi-yaml to recognize the new core config block pulumi/pulumi-yaml#369

Closed

bors bot merged commit 3bcbe35 into master Oct 18, 2022

pulumi-bot deleted the minimal-config branch October 18, 2022 17:11

AaronFriel mentioned this pull request Oct 20, 2022

Support for project level config key pulumi/pulumi-yaml#378

Closed

scottslowe mentioned this pull request Dec 9, 2022

Provider-namespace configuration values should not have a type pulumi/templates#477

Closed

	description: Implement initial MVP for hierarchical and structured project configuration
	description: Implement initial MVP for hierarchical and structured project configuration.

		@@ -986,7 +986,6 @@ func TestDestroyStackRef(t *testing.T) {

		e.RunCommand("pulumi", "up", "--skip-preview", "--yes")

		e.CWD = os.TempDir()

		@@ -1,180 +1,307 @@
		{
		"$schema": "https://json-schema.org/draft/2020-12/schema",

Hierarchical and structured config implementation: the initial pass #10832

Hierarchical and structured config implementation: the initial pass #10832

Conversation

Zaid-Ajaj commented Sep 22, 2022 • edited by Frassle

Description

Checklist

pulumi-bot commented Sep 22, 2022 • edited

Changelog

[uncommitted] (2022-10-17)

Features

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zaid-Ajaj commented Sep 24, 2022

Zaid-Ajaj commented Oct 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zaid-Ajaj commented Oct 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zaid-Ajaj commented Oct 18, 2022

bors bot commented Oct 18, 2022

Zaid-Ajaj commented Sep 22, 2022 •

edited by Frassle

pulumi-bot commented Sep 22, 2022 •

edited