-
Notifications
You must be signed in to change notification settings - Fork 18
Implement env plugin. #39
Conversation
plugins/env/payload-schema.yml
Outdated
| type: "object" | ||
| patternProperties: | ||
| .+: | ||
| type: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use pattern properties...
Just use additionalProperties: {type: string}
Also add some "description" properties, ideally with a description that can serve as documentation... It doesn't have to be long, but a line, two or 5 would be good. Usually using markdown in description properties is fine, we do it everywhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
Why must |
Because they are hard to generate... there are pros/cons... but they aren't generated by |
60727d9 to
b7bbd32
Compare
|
Implementation changed based on what @petemoore, @jonasfj and I discussed. |
| for k, v := range self.payload { | ||
| err := sandboxBuilder.SetEnvironmentVariable(k, v) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most errors here should be transformed to MalformedPayloadError see:
taskcluster-worker/engines/sandboxbuilder.go
Lines 64 to 73 in 2130156
| // Set an environement variable. | |
| // | |
| // If the format of the environment variable name is invalid this method | |
| // should return a MalformedPayloadError with explaining why the name is | |
| // invalid. | |
| // | |
| // If the environment variable have previously been declared, this method | |
| // must return ErrNamingConflict. | |
| // | |
| // Non-fatal errors: ErrFeatureNotSupported, MalformedPayloadError, |
Basically, ErrNamingConflict is a problem that can be solved by changing the task payload. The same is the case for ErrFeatureNotSupported, if env vars aren't support... user just shouldn't specify any...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors other than the ones specified as "Non-fatal errors" in
taskcluster-worker/engines/sandboxbuilder.go
Lines 64 to 73 in 2130156
| // Set an environement variable. | |
| // | |
| // If the format of the environment variable name is invalid this method | |
| // should return a MalformedPayloadError with explaining why the name is | |
| // invalid. | |
| // | |
| // If the environment variable have previously been declared, this method | |
| // must return ErrNamingConflict. | |
| // | |
| // Non-fatal errors: ErrFeatureNotSupported, MalformedPayloadError, |
Can be passed through as you have no possible way to handle those gracefully. Thus they must be fatal.
env plugin is responsible to process the "env" section from the task payload. It setups environment variables inside the sandbox.
|
|
|
This looks awesome! Tests too! ++ |
env plugin is responsible to process the section "env" from the task
payload. It setups environment variables inside the sandbox.