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 support for Krane annotations #539

Merged
merged 1 commit into from
Sep 12, 2019
Merged

Add support for Krane annotations #539

merged 1 commit into from
Sep 12, 2019

Conversation

adrianna-chang-shopify
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify commented Aug 28, 2019

What are you trying to accomplish with this PR?
Closes: #525

As described in the issue, we want to support new Krane annotations while allowing the current kubernetes-deploy annotations to remain in use.

I think we should break this down into three steps:

  1. Add support for new Krane annotations
  • kubernetes-deploy.shopify.io/timeout-override
  • kubernetes-deploy.shopify.io/prunable
  • kubernetes-deploy.shopify.io/predeployed
  • kubernetes-deploy.shopify.io/instance-rollout-conditions
  • kubernetes-deploy.shopify.io/instance-timeout
  • kubernetes-deploy.shopify.io/required-rollout
  1. Add documentation to indicate that the new annotations are preferred, and that the old ones will be deprecated
  2. (Maybe?) Raise a warning in the CLI if a kubernetes-deploy annotation is used, indicating that krane.shopify.io should be used instead

How is this accomplished?

  • Need to add support for both annotations at the same time, determining which one is being used in the template when performing validations
  • Need to duplicate tests for annotation-specific behaviour
    (Chatted with @dturn and we decided that duplicating every test that makes use of an annotated resource is unnecessary)
  • Add some docs / warnings

What could go wrong?

@RyanBrushett
Copy link
Contributor

RyanBrushett commented Sep 6, 2019

Warning looks like this

Screenshot 2019-09-06 at 10 34 17 AM

Weird that it doesn't colour the first line of the warning message as yellow.

@RyanBrushett RyanBrushett changed the title [WIP] Add support for Krane annotations Add support for Krane annotations Sep 6, 2019
Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

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

Looks on track, only big concern is DRYing it up.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
lib/kubernetes-deploy/kubernetes_resource.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/kubernetes_resource.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/kubernetes_resource.rb Show resolved Hide resolved
test/integration-serial/serial_deploy_test.rb Show resolved Hide resolved
test/unit/kubernetes-deploy/kubernetes_resource_test.rb Outdated Show resolved Hide resolved
@RyanBrushett RyanBrushett force-pushed the krane-annotations branch 2 times, most recently from 8f95924 to 3098276 Compare September 9, 2019 18:59
Copy link
Contributor

@dturn dturn left a comment

Choose a reason for hiding this comment

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

I really like how you DRY'd up the code with krane_annotation_value. I was envisioning doing the same thing with the *_annotation_key methods. Is there a reason I'm not seeing why its a bad idea?

lib/kubernetes-deploy/deploy_task.rb Outdated Show resolved Hide resolved
@RyanBrushett
Copy link
Contributor

Not at all, I went back over it and got it sorted out. Sorry for overlooking that.

Ready for another 👀

lib/kubernetes-deploy/deploy_task.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@dirceu dirceu left a comment

Choose a reason for hiding this comment

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

Just added a small question but it looks good to me!

lib/kubernetes-deploy/deploy_task.rb Show resolved Hide resolved
Copy link
Contributor

@jessie-JNing jessie-JNing left a comment

Choose a reason for hiding this comment

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

LGTM. :shipit:

- Deprecate kubernetes-deploy.shopify.io annotations
- Edit readme to reflect new annotations
- Add annotation deprecation changelog entry
- Add warning on use of depricated annotation during deploy task

co-authored-by: Ryan Brushett <ryan.brushett@shopify.com>
@RyanBrushett RyanBrushett merged commit 41a9704 into master Sep 12, 2019
@RyanBrushett RyanBrushett deleted the krane-annotations branch September 12, 2019 17:35
@douglas douglas mentioned this pull request Sep 20, 2019
@douglas douglas temporarily deployed to rubygems September 20, 2019 20:14 Inactive
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.

Add support krane.shopify.io annotations
6 participants