-
Notifications
You must be signed in to change notification settings - Fork 115
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
Conversation
d661cfc
to
5973c41
Compare
d3d3679
to
9958fd0
Compare
9958fd0
to
e8a1f0c
Compare
For this change I had to do this because after renaming It happens because in the context of any file below That is why I decided to rename |
You can explicitly tell Ruby you mean the top-level constant like this: |
TIL, thanks a lot, Katrina! I will change the code to match your suggestion ❤️ |
There was a problem hiding this 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.
e8a1f0c
to
129c853
Compare
1f0cc36
to
5a31140
Compare
5a31140
to
e852240
Compare
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 What would be the best way to proceed with this? |
I think this is looking good in general! There are a few more files we need to update:
It might make sense to voice chat about this. |
1b0f9d8
to
e852240
Compare
Great work on this massive change ~
|
We will rename the repo but not in this issue. (Probably as part of the 1.0 release issue)
The goal of not updating the gemspec in this issue is mostly to try and keep the scope reasonably sized. |
72a608a
to
6ecc2a3
Compare
9042b22
to
c80ae60
Compare
055c593
to
e3007de
Compare
There was a problem hiding this 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.
test/unit/krane/kubernetes_resource/custom_resource_definition_test.rb
Outdated
Show resolved
Hide resolved
64e8691
to
1a5b114
Compare
1a5b114
to
41f068d
Compare
What are you trying to accomplish with this PR?
Rename
kubernetes-deploy
tokrane
.Closes #527.
TODO
Fix conflicts with master
Change occurrences of
kubernetes-deploy
in the documentationDo 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
[email protected]
and rename all occurrences of[email protected]
to use the created email.Krane
How is this accomplished?
lib/kubernetes_deploy
dir tolib/krane
module KubernetesDeploy
tomodule Krane
require kubernetes_deploy/
torequire krane/
KubernetesDeploy::
toKrane::
KubernetesDeploy.
toKrane.
KubernetesDeploy
toKrane
lib/krane/cli
test/unit/krane
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 toKrane
, 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?