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

refactor(plus): Remove the spec nesting level on both input and output #347

Merged
merged 17 commits into from
Oct 19, 2020

Conversation

iliapolo
Copy link
Member

@iliapolo iliapolo commented Oct 17, 2020

Removed the need to specify various spec properties for both pre and post instantiation. Applies to all constructs that used to accept a pod spec.

Before

const deployment = new kplus.Deployment(this, text, {
  spec: {
    podSpecTemplate: {
      containers: [
        new kplus.Container({
          image: 'hashicorp/http-echo',
          args: [ '-text', text ]
        })
      ]
    }
  }
});
deployment.spec.podSpecTemplate.addContainer(...)

After

const deployment = new kplus.Deployment(this, text, {
  containers: [
    new kplus.Container({
      image: 'hashicorp/http-echo',
      args: [ '-text', text ]
    })
  ]
});
deployment.addContainer(...)

BREAKING CHANGE: spec was removed from all cdk8s+ constructs and that now have a flat structure. See Example for new usage.

  • plus: Construct id's for deployment will change due to a latent bug that appended the word pod to them.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license


this.apiObject = new k8s.Deployment(this, 'Pod', {
this.apiObject = new k8s.Deployment(this, 'Deployment', {
Copy link
Member Author

Choose a reason for hiding this comment

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

Latent copy-paste bug...this changes the generated deployment name which explains the changes in some snapshots.

@@ -122,7 +122,7 @@ function doSearch(uri, callback) {
"apiVersion": "apps/v1",
"kind": "Deployment",
"metadata": Object {
"name": "test-chart-deployment-pod-1ef542cf",
"name": "test-chart-deployment-c5d38cbe",
Copy link
Member Author

@iliapolo iliapolo Oct 17, 2020

Choose a reason for hiding this comment

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

See comment.

eladb
eladb previously approved these changes Oct 18, 2020
metadata: this.podMetadataTemplate.toJson(),
spec: this.podSpecTemplate._toKube(),
},
template: this._podTemplate._toPodTemplateSpec(),
Copy link
Contributor

Choose a reason for hiding this comment

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

toKube maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't :\

_toKube is already implemented with a different return value in PodSpec. See #347 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

oh you mean just without the underscore?

@mergify
Copy link
Contributor

mergify bot commented Oct 18, 2020

Your pull request will be updated and merged automatically (do not update manually).

/**
* @internal
*/
public _toPodTemplateSpec(): k8s.PodTemplateSpec {
Copy link
Member Author

Choose a reason for hiding this comment

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

This felt right, letting this class be responsible for creating the template spec used by Job and Deployment. It did mean I have to change the name if we want to preserve a typed return value from these methods.

We can also make this class not extend PodSpec but rather compose it and then we can use the normal _toKube, but that felt a bit redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me think we need to rename all these toKube methods to their respective output type

@mergify mergify bot dismissed eladb’s stale review October 18, 2020 10:40

Pull request has been modified.

@iliapolo iliapolo marked this pull request as ready for review October 18, 2020 10:42
@iliapolo iliapolo requested a review from eladb October 18, 2020 10:42
eladb
eladb previously approved these changes Oct 19, 2020
@mergify
Copy link
Contributor

mergify bot commented Oct 19, 2020

Your pull request will be updated and merged automatically (do not update manually).

@mergify mergify bot dismissed eladb’s stale review October 19, 2020 05:43

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Oct 19, 2020

Your pull request will be updated and merged automatically (do not update manually).

@mergify mergify bot merged commit 5e34850 into master Oct 19, 2020
@mergify mergify bot deleted the epolon/remove-spec-nesting branch October 19, 2020 07:45
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