Skip to content
This repository has been archived by the owner. It is now read-only.

Implement env plugin. #39

Merged
merged 1 commit into from Mar 9, 2016
Merged

Implement env plugin. #39

merged 1 commit into from Mar 9, 2016

Conversation

@walac
Copy link
Contributor

@walac walac commented Mar 8, 2016

env plugin is responsible to process the section "env" from the task
payload. It setups environment variables inside the sandbox.

type: "object"
patternProperties:
.+:
type: string

This comment has been minimized.

@jonasfj

jonasfj Mar 8, 2016
Contributor

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...

This comment has been minimized.

@walac

walac Mar 8, 2016
Author Contributor

Fixed.

@walac walac force-pushed the walac:master branch from 25ead5a to 24c1f60 Mar 8, 2016
@walac
Copy link
Contributor Author

@walac walac commented Mar 8, 2016

Why must generated_*.go files be checked in?

@walac walac force-pushed the walac:master branch from 24c1f60 to 6647664 Mar 8, 2016
@jonasfj
Copy link
Contributor

@jonasfj jonasfj commented Mar 8, 2016

Why must generated_*.go files be checked in?

Because they are hard to generate... there are pros/cons... but they aren't generated by go build so it's nice to do... especially because it prevents anyone from using it as a library..

@walac walac force-pushed the walac:master branch 2 times, most recently from 60727d9 to b7bbd32 Mar 8, 2016
@walac
Copy link
Contributor Author

@walac walac commented Mar 8, 2016

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

This comment has been minimized.

@jonasfj

jonasfj Mar 8, 2016
Contributor

Most errors here should be transformed to MalformedPayloadError see:

// 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...

This comment has been minimized.

@jonasfj

jonasfj Mar 8, 2016
Contributor

Errors other than the ones specified as "Non-fatal errors" in

// 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.

@walac walac force-pushed the walac:master branch from b7bbd32 to a65e733 Mar 8, 2016
Wander Lairson Costa
env plugin is responsible to process the "env" section from the task
payload. It setups environment variables inside the sandbox.
@walac walac force-pushed the walac:master branch from a65e733 to e16de29 Mar 8, 2016
@jonasfj
Copy link
Contributor

@jonasfj jonasfj commented Mar 9, 2016

r+

@petemoore
Copy link
Member

@petemoore petemoore commented Mar 9, 2016

This looks awesome! Tests too! ++

walac added a commit that referenced this pull request Mar 9, 2016
Implement env plugin.
@walac walac merged commit 1587a61 into taskcluster:master Mar 9, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.