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.

Sign up for GitHub

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

Jump to bottom

Remove kubernetes-deploy* #622

Merged
merged 13 commits into from
Nov 18, 2019
Merged

Remove kubernetes-deploy* #622

merged 13 commits into from
Nov 18, 2019

Conversation

dirceu
Copy link
Contributor

@dirceu dirceu commented Nov 12, 2019 •
edited
Loading

What are you trying to accomplish with this PR?

  • Remove the kubernetes-* executable files and their related classes.
  • Rename Krane::DeprecatedDeployTask to Krane::DeployTask
  • Remove template_dir from Krane::DeployTask
  • Remove template_dir and only_filenames from Krane::RenderTask

Note for reviewers

You can see the most important code changes in the following commits:

  • Remove template_dir and only_filenames from RenderTask
  • Remove template_dir from DeployTask
  • Remove allow_globals from DeployTask

I'll squash the commits (or at least rewrite history to make things clearer) after getting the necessary approvals.

@dirceu dirceu self-assigned this Nov 12, 2019
dturn
dturn reviewed Nov 12, 2019
View reviewed changes
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.

https://giphy.com/gifs/rl0FOxdz7CcxO/html5

lib/krane/deprecated_deploy_task.rb Outdated Show resolved Hide resolved
@dturn dturn mentioned this pull request Nov 13, 2019
Closed
8 tasks
@dirceu dirceu mentioned this pull request Nov 13, 2019
Merged
@dirceu dirceu marked this pull request as ready for review November 13, 2019 18:57
@dirceu dirceu requested a review from dturn November 13, 2019 19:06
lei-lo
lei-lo approved these changes Nov 14, 2019
View reviewed changes
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.

Nothing in the diff looks off to me, but I'm less sure if there are things that are still left to be deleted, so I defer to Danny for that. Otherwise, yay 🔥

1.0-Upgrade.md
- Removed `KubernetesDeploy#*`: use the appropriate `Krane::*` class instead.
- Removed `template_dir` from `DeployTask#initialize`: use `template_paths` instead.
- Removed `template_dir` from `RenderTask#initialize` and `only_filenames` from `RenderTask#run`: use `template_paths` instead.
- Removed `allow_globals` from `DeployTask`: use `GlobalDeployTask` instead.
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 this would be more readable in a table format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll update that after this PR is merged, as part of #619.

@dirceu dirceu requested review from dturn, timothysmith0609 and a team November 14, 2019 17:12
RyanBrushett
RyanBrushett reviewed Nov 15, 2019
View reviewed changes
Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

This is a big PR. Still digging but I wanted to add that comment for now.

lib/krane/deploy_task.rb Outdated Show resolved Hide resolved
RyanBrushett
RyanBrushett reviewed Nov 15, 2019
View reviewed changes
test/helpers/fixture_deploy_helper.rb
def deploy_global_crd_fixtures(subset:, prune: true, clean_up: true, &block)
deploy_global_fixtures("crd", subset: subset, selector: 'app=krane', clean_up: clean_up, prune: prune,
namespaced: false, &block)
def deploy_global_fixtures_non_namespaced(fixture, subset:, prune: true, clean_up: true, &block)
Copy link
Contributor

@RyanBrushett RyanBrushett Nov 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We talked IRL, might be nice to normalize the way we deploy global fixtures in our test suite cc @dirceu 👍 Perhaps we should cut an issue for it.

RyanBrushett
RyanBrushett approved these changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

Rebase and boat!

dturn
dturn reviewed Nov 15, 2019
View reviewed changes
lib/krane/deploy_task.rb Show resolved Hide resolved
lib/krane/deploy_task.rb Outdated Show resolved Hide resolved
lib/krane/deploy_task.rb
@kubectl = kubectl_instance
@max_watch_seconds = max_watch_seconds
@selector = selector
@protected_namespaces = protected_namespaces || PROTECTED_NAMESPACES
Copy link
Contributor

Choose a reason for hiding this comment

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

Should PROTECTED_NAMESPACES be the default in the function signature instead of nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing that doesn't work for some reason; a bunch of tests break. I'll check the other things and update this last if I can.

Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out this is because we explicitly pass nil from deploy_fixtures

test/integration/krane_deploy_test.rb Outdated Show resolved Hide resolved
dturn
dturn reviewed Nov 15, 2019
View reviewed changes
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.

Two small issues I'm going to push the fixes.

lib/krane/cluster_resource_discovery.rb Outdated Show resolved Hide resolved
lib/krane/deploy_task.rb Outdated Show resolved Hide resolved
@dturn dturn merged commit d1bcacb into master Nov 18, 2019
@dturn dturn deleted the remove-k8s-deploy branch November 18, 2019 02:02
@ghost ghost added the krane [ProdX-GSD] label Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dturn dturn dturn approved these changes

@RyanBrushett RyanBrushett RyanBrushett approved these changes

@lei-lo lei-lo lei-lo approved these changes

@timothysmith0609 timothysmith0609 Awaiting requested review from timothysmith0609

Assignees

@dirceu dirceu

Labels
krane [ProdX-GSD]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dirceu @RyanBrushett @dturn @lei-lo