Skip to content

Commit

Permalink
fix: remove the hardcoded 8080 for ports.
Browse files Browse the repository at this point in the history
In the Service, DeploymentConfig, Route and Health Check Enricher, the port and targetPort values were hardcoded to 8080.
This change adds the --deploy.port flag so a user can specify a port if they want.  defaults to 8080

The enrichers for the aforementioned descriptors will now use that port value

fixes #216
  • Loading branch information
lholmquist committed Apr 20, 2018
1 parent 25d084c commit bd3f10b
Show file tree
Hide file tree
Showing 13 changed files with 101 additions and 21 deletions.
5 changes: 5 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ options to create a default route, if non is provided. Defaults to false
#### removeAll
option to remove builds, buildConfigs and Imagestreams. Defaults to false - **Only for the `undeploy` Command**

#### deploy.port
Flag to update the default ports on the resource files. Defaults to 8080

#### build.recreate
Flag to recreate a BuildConfig or Imagestream. Defaults to false. Choices are "buildConfig", "imageStream", false, true. If true, both are re-created

Expand Down Expand Up @@ -202,6 +205,8 @@ Shows the below help
[default: "latest"]
--quiet supress INFO and TRACE lines from output logs
[boolean]
--deploy.port flag to update the default ports on the resource files.
Defaults to 8080 [default: 8080]
--build.recreate flag to recreate a buildConfig or Imagestream
[choices: "buildConfig", "imageStream", false, true] [default: false]
--build.forcePull flag to make your BuildConfig always pull a new image
Expand Down
5 changes: 5 additions & 0 deletions bin/nodeshift
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ yargs
type: 'boolean',
default: false
})
.options('deploy.port', {
describe: 'flag to update the default ports on the resource files. Defaults to 8080',
default: 8080
})
.option('build.recreate', {
describe: 'flag to recreate a buildConfig or Imagestream',
choices: ['buildConfig', 'imageStream', false, true],
Expand Down Expand Up @@ -131,6 +135,7 @@ function createOptions (argv) {
process.env['NODESHIFT_QUIET'] = argv.quiet === true;
options.metadata = argv.metadata;
options.build = argv.build;
options.deploy = argv.deploy;
options.cmd = argv.cmd;
// undeploy might have a positional argument
options.removeAll = argv.removeAll;
Expand Down
6 changes: 6 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ const cli = require('./bin/cli');
@param {boolean} [options.expose] - Set to true to create a default Route and expose the default service. defaults to false
@param {string} [options.nodeVersion] - set the nodeversion to use for the bucharest-gold/centos7-s2i-image. Versions are docker hub tags: https://hub.docker.com/r/bucharestgold/centos7-s2i-nodejs/tags/
@param {boolean} [options.quiet] - supress INFO and TRACE lines from output logs
@param {object} [options.deploy] -
@param {number} [options.deploy.port] - flag to update the default ports on the resource files. Defaults to 8080
@param {object} [options.build] -
@param {string/boolean} [options.build.recreate] - flag to recreate a buildConfig or Imagestream. values are "buildConfig", "imageStream", true, false. Defaults to false
@param {boolean} [options.build.forcePull] - flag to make your BuildConfig always pull a new image from dockerhub or not. Defaults to false
Expand Down Expand Up @@ -63,6 +65,8 @@ function resource (options = {}) {
@param {boolean} [options.expose] - Set to true to create a default Route and expose the default service. defaults to false
@param {string} [options.nodeVersion] - set the nodeversion to use for the bucharest-gold/centos7-s2i-image. Versions are docker hub tags: https://hub.docker.com/r/bucharestgold/centos7-s2i-nodejs/tags/
@param {boolean} [options.quiet] - supress INFO and TRACE lines from output logs
@param {object} [options.deploy] -
@param {number} [options.deploy.port] - flag to update the default ports on the resource files. Defaults to 8080
@param {object} [options.build] -
@param {string/boolean} [options.build.recreate] - flag to recreate a buildConfig or Imagestream. values are "buildConfig", "imageStream", true, false. Defaults to false
@param {boolean} [options.build.forcePull] - flag to make your BuildConfig always pull a new image from dockerhub or not. Defaults to false
Expand All @@ -84,6 +88,8 @@ function applyResource (options = {}) {
@param {string} [options.nodeVersion] - set the nodeversion to use for the bucharest-gold/centos7-s2i-image. Versions are docker hub tags: https://hub.docker.com/r/bucharestgold/centos7-s2i-nodejs/tags/
@param {boolean} [options.quiet] - supress INFO and TRACE lines from output logs
@param {boolean} [options.removeAll] - option to remove builds, buildConfigs and Imagestreams. Defaults to false
@param {object} [options.deploy] -
@param {number} [options.deploy.port] - flag to update the default ports on the resource files. Defaults to 8080
@param {object} [options.build] -
@param {string/boolean} [options.build.recreate] - flag to recreate a buildConfig or Imagestream. values are "buildConfig", "imageStream", true, false. Defaults to false
@param {boolean} [options.build.forcePull] - flag to make your BuildConfig always pull a new image from dockerhub or not. Defaults to false
Expand Down
2 changes: 1 addition & 1 deletion lib/definitions/container.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ module.exports = (resource, config) => {

const ports = [
{
containerPort: 8080,
containerPort: config.port,
name: 'http',
protocol: 'TCP'
}
Expand Down
5 changes: 1 addition & 4 deletions lib/definitions/route-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,11 @@ const _ = require('lodash');
const baseRouteSpec = {
to: {
kind: 'Service'
},
port: {
targetPort: 8080
}
};

module.exports = (resource, config) => {
resource.spec = _.merge({}, baseRouteSpec, resource.spec);
resource.spec = _.merge({}, baseRouteSpec, {port: {targetPort: config.port}}, resource.spec);

resource.spec.to.name = resource.spec.to.name ? resource.spec.to.name : config.projectName;
return resource;
Expand Down
3 changes: 3 additions & 0 deletions lib/nodeshift-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ async function setup (options = {}) {
// Return a new object with the config, the rest client and other data.
return Object.assign({}, config, {
projectPackage,
// We don't want to hard code the port later, so add it here for later use in the application descriptors
// Make sure it is a number
port: options.deploy && options.deploy.port ? parseInt(options.deploy.port, 10) : 8080,
projectName: projectPackage.name,
projectVersion: projectPackage.version || '0.0.0',
// Since we are only doing s2i builds(atm), append the s2i bit to the end
Expand Down
20 changes: 17 additions & 3 deletions lib/resource-enrichers/health-check-enricher.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,12 @@ const DEFAULT_PROBE = {
readinessProbe: {
httpGet: {
path: '/api/health/readiness',
port: 8080,
scheme: 'HTTP'
}
},
livenessProbe: {
httpGet: {
path: '/api/health/liveness',
port: 8080,
scheme: 'HTTP'
},
initialDelaySeconds: 60,
Expand All @@ -51,8 +49,24 @@ async function addHealthCheckInfo (config, resourceList) {
return resource;
}
// If so, then add the stuff
const updatedDefault = _.merge(
{},
DEFAULT_PROBE,
{
readinessProbe: {
httpGet: {
port: config.port
}
},
livenessProbe: {
httpGet: {
port: config.port
}
}
}
);
const currentContainer = resource.spec.template.spec.containers[0];
const updatedContainer = _.merge({}, DEFAULT_PROBE, currentContainer);
const updatedContainer = _.merge({}, updatedDefault, currentContainer);
resource.spec.template.spec.containers[0] = updatedContainer;

return resource;
Expand Down
4 changes: 2 additions & 2 deletions lib/resource-enrichers/service-enricher.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ function defaultService (config) {
serviceConfig.spec.ports = [
{
protocol: 'TCP',
port: 8080,
targetPort: 8080
port: config.port,
targetPort: config.port
}
];

Expand Down
4 changes: 3 additions & 1 deletion test/definitions-tests/container-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@ test('container test no container passed in', (t) => {
}
};
const config = {
projectName: 'project name'
projectName: 'project name',
port: 8080
};

const c = container(resource, config);
t.ok(c.spec.template.spec.containers, 'should have this property');
t.ok(Array.isArray(c.spec.template.spec.containers), 'should be an array');
t.equal(c.spec.template.spec.containers[0].ports[0].containerPort, 8080, 'should have port 8080');
t.end();
});

Expand Down
11 changes: 9 additions & 2 deletions test/definitions-tests/route-spec-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ test('route spec test', (t) => {
};

const config = {
projectName: 'project name'
projectName: 'project name',
port: 8080
};

const rs = routeSpec(resource, config);
t.equal(rs.spec.port.targetPort, 8080, 'targetPort should be 8080');
t.equal(rs.spec.to.kind, 'Service', 'should have a kind of Service');
t.equal(rs.spec.to.name, config.projectName, `name should be config.name ${config.projectName}`);
t.end();
Expand All @@ -24,15 +26,20 @@ test('route spec test', (t) => {
spec: {
to: {
name: 'Not Project Name'
},
port: {
targetPort: 3000
}
}
};

const config = {
projectName: 'project name'
projectName: 'project name',
port: 8080
};

const rs = routeSpec(resource, config);
t.equal(rs.spec.port.targetPort, 3000, 'targetPort should be 3000');
t.equal(rs.spec.to.kind, 'Service', 'should have a kind of Service');
t.equal(rs.spec.to.name, 'Not Project Name', `name should not be overriden and use ${resource.spec.to.name}`);
t.end();
Expand Down
14 changes: 10 additions & 4 deletions test/enricher-tests/health-check-enricher-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,16 +99,19 @@ test('health check enricher - with kube probe', async (t) => {
version: '1.0.0',
context: {
namespace: 'namespace'
}
},
port: 8080
};

const hce = await healthCheckEnricher.enrich(config, resourceList);

t.equal(Array.isArray(hce), true, 'should return an array');
t.ok(hce[0].spec.template.spec.containers[0].livenessProbe, 'should have a liveness probe added');
t.equal(hce[0].spec.template.spec.containers[0].livenessProbe.httpGet.path, '/api/health/liveness', 'should have a liveness probe url');
t.equal(hce[0].spec.template.spec.containers[0].livenessProbe.httpGet.port, 8080, 'port should be 8080');
t.ok(hce[0].spec.template.spec.containers[0].readinessProbe, 'should have a readiness probe added');
t.equal(hce[0].spec.template.spec.containers[0].readinessProbe.httpGet.path, '/api/health/readiness', 'should have a readiness probe url');
t.equal(hce[0].spec.template.spec.containers[0].readinessProbe.httpGet.port, 8080, 'port should be 8080');
t.end();
});

Expand All @@ -127,14 +130,14 @@ test('health check enricher - non default', async (t) => {
readinessProbe: {
httpGet: {
path: '/api/greeting',
port: 8080,
port: 3000,
scheme: 'HTTP'
}
},
livenessProbe: {
httpGet: {
path: '/api/greeting',
port: 8080,
port: 3000,
scheme: 'HTTP'
}
}
Expand All @@ -157,15 +160,18 @@ test('health check enricher - non default', async (t) => {
version: '1.0.0',
context: {
namespace: 'namespace'
}
},
port: 8080
};

const hce = await healthCheckEnricher.enrich(config, resourceList);

t.equal(Array.isArray(hce), true, 'should return an array');
t.ok(hce[0].spec.template.spec.containers[0].livenessProbe, 'should have a liveness probe added');
t.equal(hce[0].spec.template.spec.containers[0].livenessProbe.httpGet.path, '/api/greeting', 'url should not be overwritten');
t.equal(hce[0].spec.template.spec.containers[0].livenessProbe.httpGet.port, 3000, 'port should be 3000');
t.ok(hce[0].spec.template.spec.containers[0].readinessProbe, 'should have a readiness probe added');
t.equal(hce[0].spec.template.spec.containers[0].readinessProbe.httpGet.path, '/api/greeting', 'url should not be overwritten');
t.equal(hce[0].spec.template.spec.containers[0].readinessProbe.httpGet.port, 3000, 'port should be 3000');
t.end();
});
13 changes: 9 additions & 4 deletions test/enricher-tests/service-enricher-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ const config = {
version: '1.0.0',
context: {
namespace: 'the namespace'
}
},
port: 8080
};

test('service enricher test - no service', (t) => {
Expand All @@ -30,23 +31,25 @@ test('service enricher test - no service', (t) => {
t.equal(se[0].spec.selector.provider, 'nodeshift', 'provider should be nodeshift');
t.equal(se[0].spec.selector.project, config.projectName, `spec.selector.project should be ${config.projectName}`);
t.ok(se[0].spec.ports, 'ports prop should be here');
t.equal(se[0].spec.ports[0].port, 8080, 'port should be 8080');
t.equal(se[0].spec.ports[0].targetPort, 8080, 'targetPort should be 8080');
t.ok(Array.isArray(se[0].spec.ports), 'ports prop should be here');
t.ok(se[0].spec.type, 'type prop should be here');
t.equal(se[0].spec.type, 'ClusterIP', 'spec.type should be ClusterIP');
t.end();
});
});

test('service enricher test - no service', async (t) => {
test('service enricher test - service', async (t) => {
const resourceList = [
{
kind: 'Service',
spec: {
ports: [
{
protocol: 'TCP',
port: 8080,
targetPort: 8080
port: 3000,
targetPort: 3000
}
]
}
Expand All @@ -62,6 +65,8 @@ test('service enricher test - no service', async (t) => {
t.equal(se[0].spec.selector.provider, 'nodeshift', 'provider should be nodeshift');
t.equal(se[0].spec.selector.project, config.projectName, `spec.selector.project should be ${config.projectName}`);
t.ok(se[0].spec.ports, 'ports prop should be here');
t.equal(se[0].spec.ports[0].port, 3000, 'port should be 3000');
t.equal(se[0].spec.ports[0].targetPort, 3000, 'targetPort should be 3000');
t.ok(Array.isArray(se[0].spec.ports), 'ports prop should be here');
t.ok(se[0].spec.type, 'type prop should be here');
t.equal(se[0].spec.type, 'ClusterIP', 'spec.type should be ClusterIP');
Expand Down
30 changes: 30 additions & 0 deletions test/nodeshift-config-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ test('nodeshift-config basic setup', (t) => {
});

const p = nodeshiftConfig().then((config) => {
t.ok(config.port, 'port prop should be here');
t.equal(config.port, 8080, 'default port should be 8080');
t.ok(config.projectLocation, 'projectLocation prop should be here');
t.equal(config.projectLocation, process.cwd(), 'projectLocation prop should be cwd by default');
t.ok(config.nodeshiftDirectory, 'nodeshiftDir prop should be here');
Expand All @@ -27,6 +29,34 @@ test('nodeshift-config basic setup', (t) => {
t.equal(p instanceof Promise, true, 'should return a Promise');
});

test('nodeshift-config basic setup with deploy option', (t) => {
const nodeshiftConfig = proxyquire('../lib/nodeshift-config', {
'openshift-config-loader': () => {
return Promise.resolve({
context: {
namespace: 'test-namespace'
},
cluster: 'http://mock-cluster'
});
},
'openshift-rest-client': () => { return Promise.resolve({}); }
});

const options = {
deploy: {
port: 3000
}
};

const p = nodeshiftConfig(options).then((config) => {
t.ok(config.port, 'port prop should be here');
t.equal(config.port, 3000, 'default port should be 8080');
t.end();
}).catch(t.fail);

t.equal(p instanceof Promise, true, 'should return a Promise');
});

test('nodeshift-config other project location and nodeshiftDir', (t) => {
const nodeshiftConfig = proxyquire('../lib/nodeshift-config', {
'openshift-config-loader': () => {
Expand Down

0 comments on commit bd3f10b

Please sign in to comment.