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 first version of the migration guide to Krane 1.0 #607

Merged
merged 11 commits into from
Nov 14, 2019

Conversation

dirceu
Copy link
Contributor

@dirceu dirceu commented Oct 28, 2019

What are you trying to accomplish with this PR?

Add a migration guide to help people migrate off of kubernetes-deploy and into krane.

This can still change before 1.0, but it might be easier to maintain this guide if we merge it and update as needed (e.g. we still need to send a PR to update flag names: #602 (comment)).

@dirceu dirceu changed the title Add migration guide to Krane 1.0 Add first version of the migration guide to Krane 1.0 Nov 11, 2019
@dirceu dirceu marked this pull request as ready for review November 11, 2019 19:28
@dirceu dirceu requested a review from a team November 11, 2019 19:28
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Just a few comments so far. Will look more closely again

1.0-Upgrade.md Outdated Show resolved Hide resolved

## New task: `krane global-deploy`

`krane global-deploy` (accessible through the Ruby API as `Krane::GlobalDeployTask`) can deploy global (non-namespaced) resources such as `PersistentVolume`, `Namespace`, and `CustomResourceDefinition`. Its interface is very similar to `krane deploy`. Example usage:
Copy link

Choose a reason for hiding this comment

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

Should mention that krane deploy does not support global resources? Maybe add the same comment in the krane deploy as well?

1.0-Upgrade.md Outdated Show resolved Hide resolved
@dirceu dirceu requested a review from a team November 11, 2019 21:49
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.

Thanks for taking this on!

1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Show resolved Hide resolved
-v, --version | [none] | Replaced with `krane version`
$ENVIRONMENT | [none] | Dropped in favour of `-f`
$REVISION | --current-sha | The environment variable REVISION was dropped in favour of an explicit flag
[none] | --render-erb | **Important:** the new CLI doesn't render ERB by default
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make sure we're committed to opt-in vs totally removing it.

1.0-Upgrade.md Outdated Show resolved Hide resolved
@JennaBlack
Copy link
Contributor

Is there a difference between version 1.0 and version 1.0.0 (1 decimal vs 2) it seems to be used interchangeably throughout this doc but not really consistent

@dirceu
Copy link
Contributor Author

dirceu commented Nov 12, 2019

There shouldn't be a difference, but in any case we should make it consistent. Good shout!

1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Outdated

## Public API changes

The only breaking change in the public API is the renaming of the `KubernetesDeploy` namespace to `Krane`. Besides that, the API of the major public classes (`DeployTask`, `RenderTask`, `RunnerTask`, and `RestartTask`) didn't change at all between 0.30.0 and 1.0.0. If you're curious about this API, check the comment-based docs on these classes or the [rendered documentation at RubyGems.org](https://www.rubydoc.info/gems/kubernetes-deploy/1.0.0/KubernetesDeploy/DeployTask).
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is true, args and defaults changed. We did also change functionality by removing the ability to deploy global resources)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did args really change between kubernetes-deploy 0.30.0 and krane 1.0.0? I mean, their names will change but the default values are already different now aren't they?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the goal of the guide it help someone move from KubernetesDeploy::XTask - > Krane::XTask as well as exe/kubernetes-x -> exe/krane x . An example of a default value change would be erb rendering.

Copy link
Contributor Author

@dirceu dirceu Nov 13, 2019

Choose a reason for hiding this comment

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

That's an example of a flag default value change (covered in those flag mapping tables - although I guess we can be more explicit by having columns for old default value and new default value); the default value in the code is still false:

https://github.com/Shopify/kubernetes-deploy/blob/6c6f930cecf3fda21b6fb57cca4f7ab39455d0c9/lib/krane/deprecated_deploy_task.rb#L128-L130

My point is that people moving from KubernetesDeploy::DeployTask to Krane::DeployTask will not actually have to change much if they are on a recent version. For instance, global deploys were disabled by default in 0.30.0.

Copy link
Contributor

Choose a reason for hiding this comment

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

So it's true right now because we've intentionally forced ourselves into backwards compatibility. But, once we cut 1.0.0-pre.1 it wont be true. Are you thinking the release of 1.0.0-pre.1 will change the wording? Or the first PR to actually make a change will be responsible for updating this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or the first PR to actually make a change will be responsible for updating this?

That's what I'm thinking. I plan to have this merged and then rebase #619 and have that PR document particular changes. It might be easier to keep track of all the changes we're doing that way. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about: The only breaking change in the public API, so far, is . I don't want someone to read this today and see the 1.0 and thing ok cool. Not realizing this is a living doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Updated.

1.0-Upgrade.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lei-lo lei-lo left a comment

Choose a reason for hiding this comment

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

Thanks for writing this up! Most of my comments are of the English-is-hard type 😅

1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Show resolved Hide resolved
1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Outdated Show resolved Hide resolved
1.0-Upgrade.md Outdated Show resolved Hide resolved
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.

Small change, but looking good for the current state of the world.

1.0-Upgrade.md Show resolved Hide resolved
@dirceu dirceu requested a review from lei-lo November 13, 2019 20:14
Copy link
Contributor

@lei-lo lei-lo left a comment

Choose a reason for hiding this comment

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

🏗 1️⃣

1.0-Upgrade.md Outdated Show resolved Hide resolved
@jessie-JNing
Copy link
Contributor

Just a nit. It's a great migration guide, thanks @dirceu

@dirceu dirceu merged commit a72b4fd into master Nov 14, 2019
@dirceu dirceu deleted the add-migration-guide branch November 14, 2019 13:38
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.

5 participants