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

Rename kubernetes-deploy to krane #585

Merged
merged 16 commits into from
Oct 28, 2019
Merged

Conversation

douglas
Copy link
Contributor

@douglas douglas commented Oct 8, 2019

What are you trying to accomplish with this PR?

Rename kubernetes-deploy to krane.

Closes #527.

TODO

  • Fix conflicts with master

  • Change occurrences of kubernetes-deploy in the documentation

  • Do local tests to ensure it is working properly

  • Add Update DD dashboards task to Krane 1.0 release #528

  • Squash commits

Can be done in other moment/PRs

  • Create a mail box called krane@shopify.com and rename all occurrences of kubernetes-deploy@shopify.com to use the created email.
  • Verify TaskConfigValidatorTest#test_valid_configuration /usr/src/app/test/integration/task_config_validator_test.rb:40: warning: already initialized constant Krane::MIN_KUBE_VERSION
  • Change README (and its screenshots) to reflect the change to Krane

How is this accomplished?

  • Move files in lib/kubernetes_deploy dir to lib/krane
  • Merge lib/kubernetes-deploy/deploy_task.rb in lib/krane/deploy_task.rb
  • Rename occurrences of module KubernetesDeploy to module Krane
  • Change occurrences of require kubernetes_deploy/ to require krane/
  • Rename occurrences of KubernetesDeploy:: to Krane::
  • Rename occurrences of KubernetesDeploy. to Krane.
  • Rename remaining occurrences of KubernetesDeploy to Krane
  • Use top-level Krane constant for all files under lib/krane/cli
  • Move test/unit/kubernetes_deploy to test/unit/krane
  • Rename kubernetes_deploy_test.rb to krane_deploy_test.rb
  • Rename kubernetes_deploy_test.rb to krane_deploy_test.rb
  • Rename occurences of kubernetes-deploy in code
  • Add DeprecatedDeploy to allow global resources with a warning
  • Keep compatibility with KubernetesDeploy API
  • Still use KubernetesDeploy public api for exes, errors and integration tests

What could go wrong?

Well, I'm worried that it could contain errors that are not caught by our test suites as it moves code from KubernetesDeploy escope to Krane, which already exists.

The tests that we have now are passing, so I'm less stressed, but I would love reviews to ensure nothing breaks.

Also, is there a way to make this PR smaller or in small batches? What would be the best way go over this issue so reviews are more pleasant?

@douglas douglas self-assigned this Oct 8, 2019
@douglas douglas changed the title Rename k8s deploy to krane Rename kuberntes-deploy to krane Oct 8, 2019
@douglas douglas force-pushed the rename_k8s_deploy_to_krane branch 2 times, most recently from d3d3679 to 9958fd0 Compare October 8, 2019 21:04
@douglas douglas changed the title Rename kuberntes-deploy to krane Rename kubernetes-deploy to krane Oct 8, 2019
@douglas
Copy link
Contributor Author

douglas commented Oct 8, 2019

For this change Rename Krane::CLI::Krane to Krane::CLI::Commands:

I had to do this because after renaming KubernetesDeploy to Krane, lib/krane/cli/version_command.rb:9 is now Krane::VERSION instead of KubernetesDeploy::VERSION, but it fails with variations of this error when running tests:

Screen Shot 2019-10-08 at 5 30 02 PM

It happens because in the context of any file below lib/krane/cli/, Krane refers to Krane::CLI::Krane instead of Krane module.

That is why I decided to rename Krane::CLI::Krane to Krane::CLI::Commands, I'm not sure this is the best solution, but if you have any suggestion, I would love to know ❤️

@KnVerey
Copy link
Contributor

KnVerey commented Oct 8, 2019

You can explicitly tell Ruby you mean the top-level constant like this: ::Krane::VERSION

@douglas
Copy link
Contributor Author

douglas commented Oct 8, 2019

TIL, thanks a lot, Katrina! I will change the code to match your suggestion ❤️

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.

Great start on such a massive change. I'll 👀 again after you revert the Krane::CLI::Commands change.

We should also talk about the changes to lib/krane/deploy_task.rb. In MASTER its special cased to prevents global resources, this PR changes that functionality. I've got some ideas in #574 but we might want to do something short term.

lib/krane.rb Outdated Show resolved Hide resolved
lib/krane.rb Outdated Show resolved Hide resolved
@douglas douglas requested a review from a team October 9, 2019 15:10
@douglas douglas marked this pull request as ready for review October 9, 2019 15:13
@douglas douglas force-pushed the rename_k8s_deploy_to_krane branch 2 times, most recently from 1f0cc36 to 5a31140 Compare October 9, 2019 21:34
@douglas douglas requested a review from dturn October 9, 2019 21:35
@douglas
Copy link
Contributor Author

douglas commented Oct 9, 2019

About the Merge lib/kubernetes-deploy/deploy_task.rb in lib/krane/deploy_task.rb change:

After some reading, as Danny mentioned above, it is related to #574, therefore, I rebased with master and removed this raise "Use Krane::DeployGlobalTask to deploy global resources" if args[:allow_globals]
in https://github.com/Shopify/kubernetes-deploy/blob/master/lib/krane/deploy_task.rb#L8, so the tests could pass.

What would be the best way to proceed with this?

@dirceu
Copy link
Contributor

dirceu commented Oct 10, 2019

I think this is looking good in general! There are a few more files we need to update:

$ find . -name "*kubernetes*deploy*"
./test/unit/kubernetes-deploy
./test/integration/kubernetes_deploy_test.rb
./.shopify-build/kubernetes-deploy.yml
./lib/kubernetes-deploy
./lib/kubernetes-deploy.rb
./kubernetes-deploy.gemspec
./exe/kubernetes-deploy

We should also talk about the changes to lib/krane/deploy_task.rb. In MASTER its special cased to prevents global resources, this PR changes that functionality. I've got some ideas in #574 but we might want to do something short term.

It might make sense to voice chat about this.

@douglas douglas force-pushed the rename_k8s_deploy_to_krane branch 5 times, most recently from 1b0f9d8 to e852240 Compare October 10, 2019 14:16
@jessie-JNing
Copy link
Contributor

Great work on this massive change ~
I have two things not quite sure:

  • we are not going to rename the git repo, right? So there's no need to change the name in dev.yaml
  • about the kubernetes-deploy.gemspec, I saw a plan somewhere that we're going to do a pre-release, is that the reason we don't change the gem name here? Could anyone share more details about the plan?

@dturn
Copy link
Contributor

dturn commented Oct 10, 2019

we are not going to rename the git repo, right? So there's no need to change the name in dev.yaml

We will rename the repo but not in this issue. (Probably as part of the 1.0 release issue)

about the kubernetes-deploy.gemspec, I saw a plan somewhere that we're going to do a pre-release, is that the reason we don't change the gem name here? Could anyone share more details about the plan?

The goal of not updating the gemspec in this issue is mostly to try and keep the scope reasonably sized.

@douglas douglas force-pushed the rename_k8s_deploy_to_krane branch 3 times, most recently from 72a608a to 6ecc2a3 Compare October 11, 2019 02:31
@douglas douglas requested a review from KnVerey October 26, 2019 00:23
@douglas douglas force-pushed the rename_k8s_deploy_to_krane branch 3 times, most recently from 055c593 to e3007de Compare October 28, 2019 18:17
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

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

lib/kubernetes-deploy/delayed_exceptions.rb → lib/krane/delayed_exceptions.rb

"Renamed without changes" made me realize the code in this file isn't namespaced! Not this PR's fault, but something we should fix.

lib/krane/kubernetes_resource/custom_resource.rb Outdated Show resolved Hide resolved
lib/krane/errors.rb Outdated Show resolved Hide resolved
lib/kubernetes-deploy/rescue_krane_exceptions.rb Outdated Show resolved Hide resolved
@douglas douglas merged commit 50e7217 into master Oct 28, 2019
@douglas douglas deleted the rename_k8s_deploy_to_krane branch October 28, 2019 23:25
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.

Rename code references of kubernetes-deploy to krane
5 participants