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

Support structured config #575

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ljtfreitas
Copy link

@ljtfreitas ljtfreitas commented Apr 25, 2024

Context

Currently structured configs are not supported properly by Pulumi k8s operator.

As Pulumi programs can be configured using YAML files, it looks fair to be able to configure a structured, complex config using the Stack CRD as well.

Proposed changes

The proposed approach is to introduce a new field called configRefs, rather than change the current config one.

configRefs has a format close to other fields like envRefs and secretsRefs, supporting the same env and filesystem options as well (secret is not supported because just use secretsRefs looks better), and two new options:

  • ConfigLiteralRef
  • ConfigMapRef

ConfigLiteralRef

ConfigLiteralRef refers to both a simple or possibly structured, nested value in YAML format, in the same way that people would put in regular Pulumi.<stack>.yaml files:

configRefs:
   stack-config:
     type: Literal
     value:
        my-config:
          my-nested-field:
             field: "value"

The value content will be handled as a map of any type of value; so just simple key-pairs in YAML format are supported as well:

configRefs:
   stack-config:
     type: Literal
     value:
       field-a: value-a
       field-b: value-b

ConfigMapRef

ConfigMapRef refers to a Kubernetes ConfigMap. The implementation assumes that the content from the ConfigMap's key is a structured, nested content in YAML format. The idea is to enable people to just put their configs from Pulumi.<stack>.yaml files in a ConfigMap, and the operator will be able to handle it.

So given a ConfigMap called my-stack-config, like that:

apiVersion: v1
kind: ConfigMap
metadata:
  name: my-stack-config
data:
  Pulumi.my-stack.yaml: |
    my-config:
      my-nested-field:
          field: "value"

It could be consumed as a config to a Pulumi stack like that:

configRefs:
   stack-config:
     type: ConfigMap
     configmap:
       name: my-stack-config
       key: Pulumi.my-stack.yaml

Configs merge strategy

All configs resolved from configRefs fields will be merged with the ones from the config field. configRefs resolved key-values will override config ones with the same key.

Configs set strategy

All configs will be set using the path flag. So the config field will start to support nested values as well.

It's especially useful for some complex fields from providers. An example:

config:
   aws:assumeRole.roleArn: arn/my-aws-arn

Passing the value above with the --path flag, it will be configured like that in the result stack config:

aws:assumeRole:
  roleArn: arn/my-aws-arn

And simple key-value pairs keep working also.

Related issues (optional)

#258
#516

Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@rquitales
Copy link
Contributor

/run-acceptance-tests

@rquitales rquitales self-assigned this Apr 25, 2024
Copy link

@ljtfreitas
Copy link
Author

cc @rquitales @EronWright

@blampe
Copy link
Contributor

blampe commented Apr 25, 2024

This is awesome, thank you for the contribution! One initial thought -- still need to take a deeper look.

The idea is to enable people to just put their configs from Pulumi..yaml files in a ConfigMap, and the operator will be able to handle it.

This is a great idea. What do you think about using Pulumi's native marshalers? That would give you a little validation on top, and it might also make your flattening logic unnecessary (although I'm not 100%). The encoding package should do the trick, and you would unmarshal into a ProjectStack.

@ljtfreitas
Copy link
Author

@blampe Thanks. About your suggestion maybe I haven't expressed myself properly. My initial idea, and that's the implementation is actually doing, is to put in a ConfigMap just the config node from a Pulumi.<stack>.yaml file, not the whole ProjectStack. Pulumi.<stack>.yaml can contain fields like secretsprovider, as the struct that you mentioned have, but it was not my idea to be able to configure them through a ConfigMap.

However, it looks like a good idea to me 🤔 ; so we could move the whole stack file (Pulumi.<stack>.yaml) to a ConfigMap and we even wouldn't have to configure things like secretsprovider or backend in the CRD 🤔 ; we could just unmarshal the ConfigMap key's content as a ProjectStack. Not sure if it's related with the specific topic of structured config but looks good to me.

@blampe
Copy link
Contributor

blampe commented Apr 26, 2024

@blampe Thanks. About your suggestion maybe I haven't expressed myself properly. My initial idea, and that's the implementation is actually doing, is to put in a ConfigMap just the config node from a Pulumi.<stack>.yaml file, not the whole ProjectStack. Pulumi.<stack>.yaml can contain fields like secretsprovider, as the struct that you mentioned have, but it was not my idea to be able to configure them through a ConfigMap.

However, it looks like a good idea to me 🤔 ; so we could move the whole stack file (Pulumi.<stack>.yaml) to a ConfigMap and we even wouldn't have to configure things like secretsprovider or backend in the CRD 🤔 ; we could just unmarshal the ConfigMap key's content as a ProjectStack. Not sure if it's related with the specific topic of structured config but looks good to me.

Ah, I had misunderstood what all was in the ConfigMap, I didn't realize it was just the config key. In that case you could marshal the config.Map instead of the whole ProjectStack as I had suggested earlier. I didn't mean to increase the scope of your change!

@ljtfreitas
Copy link
Author

@blampe Don't worry, maybe I haven't expressed myself properly in the PR description (English is not my first language so sometimes talking about complex stuff is just...complex 😅). About your suggestion, I'm trying to refactor the code to use config.Map but I'm struggling to read the content. When I try to unmarshal I see this error:

could not parse <my-config> as a configuration key (configuration keys should be of the form `<namespace>:<name>`

Looking at the code that deserializes a YAML to a ProjectStack instance (in the Automation API code), config keys are namespaced (with the project name) before the config.Map's unmarshal 😕 ...and I can't use the same function because it's private. So config keys must be namespaced in the ConfigMap.

Not sure if we need to enforce that here because this behavior will be applied anyway during the config-set phase (and speaking for myself I don't namespace my configs because of this)

@ljtfreitas
Copy link
Author

hey friends, any news here? we really need this feature

@blampe @rquitales

@blampe
Copy link
Contributor

blampe commented May 9, 2024

Sorry for the delay @ljtfreitas, I plan on playing around with this tomorrow!

Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Apologies again for the delay. I was able to spend some hands-on time with this and have a much better feel for things.

The biggest issue I see is that I think we'll want ConfigRefs to be an array. Let me know if you think that sounds reasonable.

I'm still playing around with some of the config merging logic locally, in case there's some room to simplify things.

@@ -0,0 +1,90 @@
module structured-config-refs
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, did you include a Go program here because you're using Go for your own projects?

I'm a big fan of YAML projects for tests, but this is fine.

Copy link
Author

Choose a reason for hiding this comment

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

I guess I was a bit biased here in fact 😅, I wrote the example/test program in Go because I wanted to be sure that we could deserialize a structured, complex config in the way that we usually do in Go

@@ -14,6 +15,7 @@ import (

"github.com/pulumi/pulumi-kubernetes-operator/pkg/apis/pulumi/shared"
pulumiv1 "github.com/pulumi/pulumi-kubernetes-operator/pkg/apis/pulumi/v1"
"sigs.k8s.io/yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 We've had issues with marshaling when not using the k8s fork, so this is good.

description: (optional) ConfigRefs is the configuration for this stack,
which can be specified through ConfigRef. If this is omitted, configuration
is assumed to be checked in and taken from the source repository.
If present, ConfigRefs values will be merged with values passed
Copy link
Contributor

Choose a reason for hiding this comment

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

We should clarify which one has precedence -- e.g. ConfigRefs will overwrite Configs.

Copy link
Author

@ljtfreitas ljtfreitas May 14, 2024

Choose a reason for hiding this comment

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

but ConfigRefs will not overwrite Configs, right? in fact both fields will be merged

I guess the description could be improved to explain the ConfigRefs will overwrite Configs with the same key name

Comment on lines 84 to 90
additionalProperties:
description: ConfigRef identifies a resource from which config information
can be loaded. Environment variables, files on the filesystem,
Kubernetes Secrets, ConfigMaps, structured and config literal
values strings are currently supported.
properties:
configmap:
Copy link
Contributor

Choose a reason for hiding this comment

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

ConfigRefs will need to be an array to allow supporting more than one ref with predictable precedence.

For example the user might want to specify more than one secret ref, and if each of those refs contains the same key, then the value from the last ref should win.

As a map, the user can't specify the same ref type more than once, and the map's iteration order isn't guaranteed.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting. It could be an array, but do we really need this? 🤔 ConfigRefs would have the same semantics as EnvRefs field.

In EnvRefs, one could specify more than one secret, or more than one env var, with the same key name and the order will not be guaranteed; but this would look like a misconfiguration to me (using the same secret key twice), the behavior seems reasonable.

also, the ConfigRefs spec above supports to specify the same ref type more than once, like:

configRefs:
   first-secret:
      type: Secret
      secret:
        name: my-secret
        key: key-name
   second-secret:
      type: Secret
      secret:
        name: my-another-secret
        key: key-name

Copy link
Contributor

@blampe blampe May 15, 2024

Choose a reason for hiding this comment

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

EnvRefs makes sense as a map because the keys are just variable names, and I agree you would never need to specify the same variable twice.

ConfigRefs is different because each ref can potentially be its own bag of key/values. All of those key/values need to be coalesced and we want that process to be well-defined.

also, the ConfigRefs spec above supports to specify the same ref type more than once, like:

Yes, you're right, I wasn't clear on the key when I wrote that. I'm still worried about the iteration order, though. Consider the case where you have some "base" config that you want individual stacks to be able to override:

configRefs:
   base:
     type: Structured 
     value:
       my-config: base-value
   override:
      type: Structured
      value:
        my-config: override-value

I would expect the override value to always be applied, but that's not guaranteed.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, it makes sense. I will change it 👍

Copy link
Author

@ljtfreitas ljtfreitas May 16, 2024

Choose a reason for hiding this comment

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

Thinking about the best way to do it 🤔 ...maybe an "array of maps"?

configRefs:
   - base:
        type: Structured 
        value:
           my-config: base-value
   - override:
        type: Structured
        value:
           my-config: override-value

Or an object with a "name" field (or "key" or something)?

configRefs:
   - name: base
     type: Structured 
     value:
         my-config: base-value
   - name: override
     type: Structured
     value:
         my-config: override-value

I prefer the first one but any thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

@blampe just added a new test case for this scenario

Comment on lines +1651 to +1653
sort.Slice(configValues, func(i, j int) bool {
return configValues[i].Key < configValues[j].Key
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to double check this, but I believe keys can be specified in flat or exploded form, so for example object.nums[0]: 1 and object: { nums: [1] }. I think you already handle this by flattening, but it would be worth a test to verify that 2 refs specifying the same key in different ways will be merged together.

Copy link
Author

@ljtfreitas ljtfreitas May 14, 2024

Choose a reason for hiding this comment

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

Is the second example a valid syntax? As a json?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just doubled checked -- these are two ways of describing the same thing:

  stack:deepobject:
    nums:
      - 1
  stack:flatobject: '{"nums": [1]}

It doesn't look like stack:object.nums[0]: 1 is valid.

I think it would be totally fine to say this is an edge case we don't handle, given that it's unlikely to come up.

@@ -205,6 +207,386 @@ var _ = Describe("Stack Controller", func() {
})
})

Context("configuring a stack using ConfigRefs", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you enormously for adding tests!

Stack: stackName,
GitSource: &shared.GitSource{
ProjectRepo: baseDir,
RepoDir: "test/testdata/config-refs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just test/testdata/structured-config-refs for all of these? This is testing we can read the configs, seems worth also confirming the structure was handled correctly.

Copy link
Author

Choose a reason for hiding this comment

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

Actually this test is trying to read a file, and in a similar way that I implemented with Literal, I used the same existing behavior to FileRef (the operator already use it to load envvars and secrets from files), which means that just loads a single value. So this test couldn't use test/testdata/structured-config-refs

Do you believe that would be worth implementing a file behavior specifically to load configs? So we could support structured values as well.

structuredConfig := StructuredConfig(configMapContent).Flatten()
allConfigs = append(allConfigs, structuredConfig...)
}
case shared.ConfigResourceSelectorStructured:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused about the need for both "structured" and "literal".

The legacy resolveResourceRef logic doesn't do any special handling for "literal", but we have an opportunity to be more flexible here.

In other words, it's odd to have two ways to express the same thing:

configRefs:
   stack-config:
     type: Literal
     value: foo
--
configRefs:
   stack-config:
     type: Structured
     value: foo

But this is net-new:

configRefs:
   stack-config:
     type: Structured
     value:
        my-config:
          my-nested-field:
             field: "value"

It would seem reasonable IMO to basically remove structured and just change how we handle literals (basically replacing this block with case ResourceSelectorLiteral).

Copy link
Author

Choose a reason for hiding this comment

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

I thought about this option first but the reason why I've preferred to introduce the "structured" type was don't change the "literal" behavior, because it's already used by EnvRefs and SecretRefs fields, and for these use cases the "literal" option looks good enough (specially for env var case)

For these use cases just passing a literal, simple string value makes sense, but a structured, complex value does not, I think...so I didn't want to change that, and introduced the new "structured" option (I used this word because Pulumi docs also use it to reference about complex configs. So the main reason was backward compatibility.

but you're right, it sounds a bit strange. maybe we could handle "literal" for configs as you suggested and also keep the legacy "literal" behavior for the other Refs fields. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we don't want to change literal handling for EnvRefs or SecretRefs. (There's an argument that we should, but we don't need to think about that as part of this feature.)

but you're right, it sounds a bit strange. maybe we could handle "literal" for configs as you suggested and also keep the legacy "literal" behavior for the other Refs fields. WDYT?

Yeah, that's what I was thinking. Let EnvRefs and SecretRefs continue to use resolveResourceRef, and you can use your structure handling for literals here.

@ljtfreitas
Copy link
Author

ljtfreitas commented May 14, 2024

thank you @blampe, don't worry, don't need to apologize, thank you for your support (i hope that I have not sounded like a jerk or something 😅 also), i will check the comments 👀

Copy link

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@ljtfreitas ljtfreitas requested a review from blampe May 20, 2024 19:59
@ljtfreitas
Copy link
Author

hi friends 👋 , any news here? cc @blampe

@miguelslemos
Copy link

Hi @blampe, this feature is crucial for us, could you take a look when you get time, please?

Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

Sorry, have been OOO. I'm still a little uncertain about the type of ConfigRefs, and I worry we're duplicating some of SecretRefs. It would be nice to test merging behavior when keys collide.

Hi @blampe, this feature is crucial for us, could you take a look when you get time, please?

You should be able to use a custom image if this is blocking you.

allConfigs = append(allConfigs, configs...)
}
// Secret should be handled here as well because auto.ConfigValue should be marked as Secret:true
case shared.ConfigResourceSelectorType(shared.ResourceSelectorSecret):
Copy link
Contributor

Choose a reason for hiding this comment

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

When would I use this instead of the existing SecretRefs?

Copy link
Author

Choose a reason for hiding this comment

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

My original idea was to support the same options from ResourceRef but I'm wondering about that...maybe should we support just literal/structured values and ConfigMap for configs?

Comment on lines +46 to +56
sourceConfigMap := map[string]any{
"aws:assumeRole": map[string]any{
"roleArn": "my-role-arn",
"sessionName": "my-session-name",
},
"aws:defaultTags": map[string]any{
"tags": map[string]any{
"my-tag": "tag-value",
},
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For what it's worth, I think if you add another key like "aws:assumeRole.roleArn": "other-role-arn" this will fail non-deterministically.

Copy link
Author

Choose a reason for hiding this comment

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

Could be but, sorry, is it a real problem? For a stack configured like this,

aws:assumeRole:
   roleArn: "my-role-arn"
aws:assumeRole.roleArn: "other-role-arn"

I wouldn't expect any deterministic behavior here, it would look like a configuration mistake to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know it may seem weird, but it's possible so we need to consider it. Hyrum's law.

It would be very helpful to see some more tests around what happens when multiple config maps are provided with overlapping keys like this.

Comment on lines 1091 to 1092
for _, configRefMap := range sess.stack.ConfigRefs {
for k, ref := range configRefMap {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not clear on why this is a []map[string]ConfigRef instead of just a []ConfigRef. The key only seems to be used for secret refs (which mirrors SecretRefs?), and for the default case (do we need that?).

Copy link
Author

Choose a reason for hiding this comment

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

It was a design decision, that I've mentioned here: #575 (comment)

Copy link
Author

@ljtfreitas ljtfreitas Jun 3, 2024

Choose a reason for hiding this comment

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

I've chosen the array of maps option to be similar to the envRefs and secretRefs fields, where the key is the env/secret key name.

But in fact for structured/config map cases the key is not used 😕...not sure what could be a better option. wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

For literal values it's used as the config-key name:

configRefs:
   - sample:
        type: Literal
        value: just-a-value

it will be resolved as a config like that:

sample: just-a-value

Comment on lines +1141 to +1142
default:
// try to resolve as a ResourceRef
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? resolveResourceRef doesn't support structure so it seems odd to default to it. An error would be reasonable IMO.

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't, but resolveResourceRef supports all other cases like env, file, etc. These options are supported in configRefs too.

Maybe it does not make sense and configRefs should support just structured/config map options?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should keep the new API surface area as small as possible. Unless we have a good reason to include them now, we can omit env, file etc. and add them later if needed. We don't need a key k for the literal case which leaves this default case as the only reason for it. Not needed IMO.

Copy link

github-actions bot commented Jun 3, 2024

PR is now waiting for a maintainer to run the acceptance tests. This PR will only perform build and linting.
Note for the maintainer: To run the acceptance tests, please comment /run-acceptance-tests on the PR

@ljtfreitas
Copy link
Author

hey @blampe , I've made some changes and updated the PR description; I've removed the structured concept and started to handle just as literal values, both simple or complex ones.

@ljtfreitas ljtfreitas requested a review from blampe June 3, 2024 23:36
Copy link
Contributor

@blampe blampe left a comment

Choose a reason for hiding this comment

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

I think we can still simplify the API a bit. Let's try to scope this down to only the things we know we need, which it sounds like is config maps and literals?

Other things that will help get this over the line:

  • Blocker: Resolve non-determinism by making ConfigRefs an array.
  • Blocker: Tests around edge cases with merging multiple refs/literals.
  • Optional: We could limit the change to an alpha API, to leave room for changes before we add it to v1.

Comment on lines +86 to +87
If this is omitted, configuration is assumed to be checked in and
taken from the source repository. If present, ConfigRefs values
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we over-write in-repo configs entirely? Why not overlay these configs on top of source?

It seems surprising to be able to commit some changes to config, deploy it, but then not see the new config take effect. Is this the existing behavior of the operator?

Comment on lines +109 to +113
namespace:
description: Namespace where the ConfigMap is stored.
Deprecated; non-empty values will be considered invalid
unless namespace isolation is disabled in the controller.
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not add new things that are already deprecated.

Suggested change
namespace:
description: Namespace where the ConfigMap is stored.
Deprecated; non-empty values will be considered invalid
unless namespace isolation is disabled in the controller.
type: string

Comment on lines +119 to +120
description: Env selects an environment variable set on the
operator process
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like this was copy-pasted, let's make sure the doc strings are accurate.

Comment on lines +152 to +153
of selector. Must be one of: Env, FS, Secret, ConfigMap,
Structured, Literal'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
of selector. Must be one of: Env, FS, Secret, ConfigMap,
Structured, Literal'
of selector. Must be one of: Env, FS, ConfigMap, or Literal.'

Or just ConfigMap/Literal if you scope it down to those two.

@@ -898,6 +975,83 @@ spec:
which can be optionally specified inline. If this is omitted, configuration
is assumed to be checked in and taken from the source repository.
type: object
configRefs:
Copy link
Contributor

Choose a reason for hiding this comment

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

It was suggested that we keep this limited to an alpha version like this before adding it to v1. That way we aren't immediately locked in to the API.

@@ -1085,6 +1086,64 @@ func (sess *reconcileStackSession) SetEnvRefsForWorkspace(ctx context.Context, w
return nil
}

func (sess *reconcileStackSession) resolveConfigRefs(ctx context.Context) ([]ConfigKeyValue, error) {
allConfigs := make([]ConfigKeyValue, 0)
for _, configRefMap := range sess.stack.ConfigRefs {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still non-deterministic, and the name is still unused. Please make this an array.

Value auto.ConfigValue
}

func NewStructuredConfigFromJSON(key string, rawValue apiextensionsv1.JSON) (*StructuredConfig, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to accept YAML?

Comment on lines +46 to +56
sourceConfigMap := map[string]any{
"aws:assumeRole": map[string]any{
"roleArn": "my-role-arn",
"sessionName": "my-session-name",
},
"aws:defaultTags": map[string]any{
"tags": map[string]any{
"my-tag": "tag-value",
},
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it may seem weird, but it's possible so we need to consider it. Hyrum's law.

It would be very helpful to see some more tests around what happens when multiple config maps are provided with overlapping keys like this.

literalRef := configRef.Literal
if literalRef != nil {
// ConfigLiteralRef handles both simple and structured values as json, flattening all keys to build a list of Pulumi key:value configs
structuredConfig, err := NewStructuredConfigFromJSON(k, literalRef.Value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Already mentioned above, but interpreting literal as an object eliminates the need for k while still allowing structure.

literal:
  value:
    aws:region: us-west-2
    foo:bar:
      - baz

Comment on lines +1141 to +1142
default:
// try to resolve as a ResourceRef
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should keep the new API surface area as small as possible. Unless we have a good reason to include them now, we can omit env, file etc. and add them later if needed. We don't need a key k for the literal case which leaves this default case as the only reason for it. Not needed IMO.

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.

None yet

4 participants