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

Make task arguments match the new flag names #619

Merged
merged 13 commits into from
Nov 18, 2019
Merged

Make task arguments match the new flag names #619

merged 13 commits into from
Nov 18, 2019

Conversation

dirceu
Copy link
Contributor

@dirceu dirceu commented Nov 11, 2019 •
edited
Loading

What are you trying to accomplish with this PR?

  • Make sure task arguments match the new flag names so we have a consistent interface both in the API and in the CLI (Global deploy 2 #602 (comment)).
  • Be explicit about which arguments are required.

How is this accomplished?

By changing the public task interfaces so their argument names would match the respective flag names, and updating the test setup accordingly.

@dirceu dirceu self-assigned this Nov 11, 2019
@dirceu dirceu changed the title Make sure task arguments match the new flag names Make task arguments match the new flag names Nov 11, 2019
@dirceu dirceu marked this pull request as ready for review November 12, 2019 16:31
@dturn dturn mentioned this pull request Nov 13, 2019
Closed
8 tasks
@dirceu dirceu requested review from a team and dturn November 13, 2019 13:49
lei-lo
lei-lo reviewed Nov 13, 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.

Mostly looks good, just a couple questions from me! ❓

exe/kubernetes-restart Outdated Show resolved Hide resolved
@dirceu dirceu requested a review from lei-lo November 13, 2019 16:02
dturn
dturn reviewed Nov 13, 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.

Added some thoughts about consistency across tasks as well

lib/krane/cli/deploy_command.rb Show resolved Hide resolved
lib/krane/cli/deploy_command.rb
bindings: bindings_parser.parse,
logger: logger,
max_watch_seconds: ::Krane::DurationParser.new(options["global-timeout"]).parse!.to_i,
global_timeout: ::Krane::DurationParser.new(options["global-timeout"]).parse!.to_i,
selector: selector,
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.

I can't comment on line 73. But should allow_protected_ns: !protected_namespaces.empty?, be renamed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually could be removed from the public interface, right? This is maintained as an argument mostly for backwards compatibility with kubernetes-deploy.

Copy link
Contributor

Choose a reason for hiding this comment

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

allow_protected_ns can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will remove on #622.

lib/krane/cli/restart_command.rb Outdated Show resolved Hide resolved
lib/krane/cli/run_command.rb Show resolved Hide resolved
@dirceu dirceu force-pushed the update-arg-names branch 4 times, most recently from 9a0f7af to 6bbb909 Compare November 13, 2019 19:38
@dirceu dirceu mentioned this pull request Nov 13, 2019
Merged
@dirceu
Copy link
Contributor Author

dirceu commented Nov 14, 2019

Added a commit with changes to the migration guide.

@dirceu dirceu requested a review from dturn November 14, 2019 13:50
dturn
dturn reviewed Nov 14, 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.

I think we need to merge #622, then rebase this before a final review. Otherwise I fear we're going to miss something. (In general its looking good).

lib/krane/restart_task.rb
# @param selector [Hash] Selector(s) parsed by Krane::LabelSelector
# @param verify_result [Boolean] Wait for completion and verify success
#
# @return [nil]
def run!(deployments_names = nil, selector: nil, verify_result: true)
def run!(deployments: nil, selector: nil, verify_result: true)
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 deployments is the right name here. There was talk about supporting DS's as well, under a new keyword. I think that's better than a generic resources where we have to manually fetch both and check to see what exists.

lib/krane/render_task.rb Outdated
@@ -12,13 +12,13 @@ class RenderTask
#
# @param logger [Object] Logger object (defaults to an instance of Krane::FormattedLogger)
# @param current_sha [String] The SHA of the commit
# @param template_dir [String] Path to a directory with templates to render (deprecated)
# @param template_paths [Array<String>] An array of template paths to render
# @param template_dir [String] Path to a directory with templates to render (*deprecated*)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you 🔥 this in a different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, on #622

@dirceu dirceu mentioned this pull request Nov 14, 2019
Merged
@dturn dturn changed the base branch from master to remove-k8s-deploy November 16, 2019 01:27
@tsontario
Copy link

WDYT about the fact we still have OptionsHelper::with_process_template_paths and TemplateSets. Do these terms still make sense with the filename(s) semantics?

@dirceu
Copy link
Contributor Author

dirceu commented Nov 16, 2019 •
edited
Loading

Thanks for the rebase! I think it makes sense; I'll just send a separate commit bringing back the information about the removed arguments in the tables we're using to describe the renames.

WDYT about the fact we still have OptionsHelper::with_process_template_paths and TemplateSets. Do these terms still make sense with the filename(s) semantics?

Hmmm... well, the filename is the name of the file containing a template, so I think that TemplateSets still make sense. We could arguably change the OptionsHelper method but I wouldn't consider this a blocker for this issue, as our main goal here is to update the public interface of the Task classes. Unless you feel strongly that we should change this in this PR, I'd rather make this change afterwards.

@dturn
Copy link
Contributor

dturn commented Nov 17, 2019

We could arguably change the OptionsHelper method but I wouldn't consider this a blocker for this issue, as our main goal here is to update the public interface of the Task classes. Unless you feel strongly that we should change this in this PR, I'd rather make this change afterwards.

Added to the tech debt tracking issue:
#637

dturn
dturn reviewed Nov 17, 2019
View reviewed changes
lib/krane/cli/run_command.rb Outdated
@@ -19,10 +19,11 @@ class RunCommand
},
"verify-result" => { type: :boolean, desc: "Wait for completion and verify pod success", default: true },
"command" => { type: :array, desc: "Override the default command in the container image" },
"template" => {
"filename" => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we decided against this #568

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thank you! I wasn't aware of that. I'll update this PR undoing this particular change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. This is ready for another review.

@dirceu dirceu force-pushed the update-arg-names branch 2 times, most recently from 18b841f to 564c3cc Compare November 18, 2019 00:16
@dturn dturn changed the base branch from remove-k8s-deploy to master November 18, 2019 02:01
@dturn dturn merged commit 4b9f93c into master Nov 18, 2019
@dturn dturn deleted the update-arg-names branch November 18, 2019 02:24
@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

@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
5 participants
@dirceu @tsontario @dturn @lei-lo @timothysmith0609