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

Support more types #57

Closed
wants to merge 5 commits into from
Closed

Support more types #57

wants to merge 5 commits into from

Conversation

ankon
Copy link
Contributor

@ankon ankon commented Jan 11, 2017

Relates to #53

Copy link
Contributor

@silasbw silasbw left a comment

Choose a reason for hiding this comment

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

Awesome stuff. Can you add tests that exercise this new code? There's examples in 1 for writing integration and unit tests. It might make sense to split this up for each new API group, especially if you add tests.

];
options = Object.assign({}, options, {
path: 'apis/apps',
version: options.version || 'v1beta1',
Copy link
Contributor

Choose a reason for hiding this comment

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

How should we handle the petset -> statefulset rename 1?

The simplest thing would be to add statefulset to the genericTypes array and add a comment about the rename. Any better ideas?

];
options = Object.assign({}, options, {
path: 'apis/batch',
version: options.version || 'v2alpha1',
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a v1 batch -- should we make that the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really good question, I was thinking about that as well.

I guess it makes sense to have the "conservative" default here, anyone that wants alpha/beta stuff should do so knowingly.

class Batch extends ApiGroup {
constructor(options) {
const genericTypes = [
'scheduledjobs'
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar rename issue as above -- scheduledjobs -> cronjobs 1

@@ -5,8 +5,11 @@ module.exports.aliasResources = function (resourceObject) {
// http://kubernetes.io/docs/user-guide/kubectl-overview/
// and anything else we think is useful.
const resourceAliases = {
clusterroles: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

should we include petsets and scheduledjobs?

@@ -37,7 +37,10 @@ class Namespaces extends BaseObject {
// Generic objects we don't implement special functionality for.
//
const genericTypes = [
'clusterroles',
Copy link
Contributor

Choose a reason for hiding this comment

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

should we include petsets and scheduledjobs?

@@ -2,9 +2,15 @@

const Core = require('./core');
const Extensions = require('./extensions');
const Apps = require('./apps');
const Batch = require('./batch');
const RBAC = require('./rbac');
Copy link
Contributor

Choose a reason for hiding this comment

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

Our convention has been to avoid consecutive caps (e.g., apiUrl, not apiURL). So this would be Rbac not RBAC. Rbac looks funny, I know, but it follows the convention we're currently using.

This makes it possible to work with apps/v1beta1.StatefulSet instances
using the kubernetes-client.

This includes support for the deprecated name apps/v1alpha1.PetSet.
This also includes support for the old name 'ScheduledJob'.
@ankon
Copy link
Contributor Author

ankon commented Jan 12, 2017

I've fixed the nits above and pushed for a quick look at it - but tests are still missing and coming.

This is now rebased this onto #59, while I'm working on the tests :)

@silasbw
Copy link
Contributor

silasbw commented Jan 26, 2017

Did you make progress on tests? or get waylaid by that cleanup that needed to happen :)

Is there something I can do to help you finish this?

@silasbw
Copy link
Contributor

silasbw commented Feb 18, 2017

Rebased/updated branch here: #78

Thanks again @ankon for initiating these changes.

@silasbw silasbw closed this Feb 18, 2017
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.

None yet

2 participants