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

Add node_envvars module, #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add node_envvars module, #29

wants to merge 1 commit into from

Conversation

arishpyne
Copy link

This module adds an ability to set environment variables on Jenkins nodes

Copy link

@akaRem akaRem left a comment

Choose a reason for hiding this comment

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

Please

title: Schema for Jenkins Global EnvVars setter
type: object
minProperties: 1
additionalProperties:
Copy link

Choose a reason for hiding this comment

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

Could you please look here https://spacetelescope.github.io/understanding-json-schema/reference/object.html#pattern-properties ?
AFAIU, hee should be KV with UpperString to string mapping. But now schema allows to pass something like

1: {"foo": {"bar": ["baz" ,2]}}

Copy link
Author

Choose a reason for hiding this comment

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

I believe that using

  additionalProperties:
    type: string

is actually enough to ensure that the value must be a string.
This is shown by the last example on the doc page you've linked.

I've also made a gist to illustrate this.

Actually, while I've reviewed the schema I've suddenly realized that setting the minimum of one child element for a config to be considered valid will lead to an inability to disable the environment variables at all.
This makes {node-envvars: {"node1": []}} invalid, while it represents a valid state of environment variables. I'm going to fix that on both PRs.

jenkins_yaml_path: '\n'.join(
[
'jenkins:',
' slave-envvars:',
Copy link

Choose a reason for hiding this comment

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

Something is incorrect here.. You provided example with "node-env-vars", but here "slave-envvars" is used. This is probably because of too permissive schema.
BTW IMHO "node-env-vars" sounds better.

@@ -0,0 +1,6 @@
title: Schema for Jenkins Global EnvVars setter
type: object
additionalProperties:
Copy link

Choose a reason for hiding this comment

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

@akaRem
Copy link

akaRem commented Oct 20, 2017

Sorry, looks like a couple of my comments were corrupted

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.

2 participants