Skip to content
This repository was archived by the owner on Feb 27, 2020. It is now read-only.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@walac
Copy link
Contributor Author

walac commented Mar 8, 2016

Why must generated_*.go files be checked in?

@jonasfj
Copy link
Contributor

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 master branch 2 times, most recently from 60727d9 to b7bbd32 Compare March 8, 2016 23:24
@walac
Copy link
Contributor Author

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
Copy link
Contributor

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:

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

Copy link
Contributor

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

// 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.
@jonasfj
Copy link
Contributor

jonasfj commented Mar 9, 2016

r+

@petemoore
Copy link
Member

This looks awesome! Tests too! ++

walac added a commit that referenced this pull request Mar 9, 2016
@walac walac merged commit 1587a61 into taskcluster:master Mar 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants