Make environment variable population more generic #125
Conversation
| "aws:region": "us-east-2", | ||
| }, | ||
| Stack: stackName, | ||
| ProjectRepo: "https://github.com/metral/test-s3-op-project", // TODO: relocate to some other repo |
There was a problem hiding this comment.
This is a reminder - we should probably open an issue to track migrating these test cases to a Pulumi repo.
There was a problem hiding this comment.
Agreed. Cut #126 to track this.
deploy/crds/pulumi.com_stacks.yaml
Outdated
| @@ -57,6 +57,11 @@ spec: | |||
| items: | |||
| type: string | |||
| type: array | |||
| envSecretsFromPath: | |||
There was a problem hiding this comment.
These are strictly secrets, are they? In the sense that the other uses of these he word Secret here are about K8s Secret objects.
Are there other precedents for naming for things like this in other operators we could model after? Not sure if this pattern has common terminology associated with it?
There was a problem hiding this comment.
Fair point. I was positioning this as an alternative to envSecrets. It can just be called envFromPath instead.
I was also wondering if it makes sense to take a slightly different tack on this by modeling things as a reference instead. For instance for #97 I can foresee us wanting to support literal, secret reference, environment variable or a file as well and could reuse the same model. As a result I am proposing adding a ResourceRef type. Other operators seem to take a similar approach.
Update - I have switched to this model so the result is a bit more generic and reusable.
deploy/crds/pulumi.com_stacks.yaml
Outdated
| type: string | ||
| required: | ||
| - name | ||
| - namespace |
There was a problem hiding this comment.
Seems like namespace could be optional.
|
|
||
| // ResourceRef identifies a resource from which information can be loaded. | ||
| type ResourceRef struct { | ||
| // SelectorType is required and signifies the type of selector. Should be one of: |
There was a problem hiding this comment.
| // SelectorType is required and signifies the type of selector. Should be one of: | |
| // SelectorType is required and signifies the type of selector. Must be one of: |
viveklak
changed the title
|
@lukehoban @lblackstone sorry for the noise. Could you take another look? I went ahead with the approach I proposed in #125 (comment) |
| } else { | ||
| return errors.Errorf("Mising secret reference in ResourceRef for '%s'", envVar) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should add a default case that returns error just to be safe.
| envVarVal := os.Getenv(ref.Env.Name) | ||
| w.SetEnvVar(envVar, envVarVal) | ||
| } else { | ||
| return errors.Errorf("Missing env reference in ResourceRef for '%s'", envVar) |
There was a problem hiding this comment.
nit: This syntax is slightly cleaner
| return errors.Errorf("Missing env reference in ResourceRef for '%s'", envVar) | |
| return errors.Errorf("Missing env reference in ResourceRef for %q", envVar) |
test/stack_controller_test.go
Outdated
| return fetched.Status.LastUpdate.LastSuccessfulCommit == stack.Spec.Commit && | ||
| fetched.Status.LastUpdate.LastAttemptedCommit == stack.Spec.Commit && | ||
| fetched.Status.LastUpdate.State == pulumiv1alpha1.SucceededStackStateMessage |
There was a problem hiding this comment.
If this is done commonly, it would be good to expose as a function. Fine for this test, if not.
Fixes #122