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 ResolvePodSpec to podspec.go and move the related utilities to podspec_helper #1024

Merged
merged 5 commits into from
Oct 27, 2020

Conversation

daisy-ycguo
Copy link
Member

@daisy-ycguo daisy-ycguo commented Sep 22, 2020

Description

It's a refactor PR. The goal is to add a method

func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, cmd *cobra.Command) error

in flags/podspec.go so that the logic to resolve a PodSpec from flags can be used by both service commands group and ContainerSource commands group.

This PR demonstrates how PodSpecFlags.ResolvePodSpec is used in service commands. PR #1016 demonstrates how PodSpecFlags and PodSpecFlags.ResolvePodSpec is used in container source commands.

Changes

  • Add a method ResolvePodSpec in flags/podspec.go
  • Move podspec related utilities from serving/config_changes.go to flags/podspec_helper.go
  • Move two common functions from serving/config_changes.go to utils/corev1_helper.go
  • Refactor service commands to adopt the changes

Reference

Refer to PR #1016 and see how PodSpecFlags.ResolvePodSpec is used in container source commands group.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 22, 2020
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2020
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 22, 2020
@daisy-ycguo daisy-ycguo changed the title Moving podspec related utilities to podspec_helper Add ResolvePodSpec to podspec.go and move the related utilities to podspec_helper Sep 22, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2020
pkg/kn/flags/podspec.go Outdated Show resolved Hide resolved
Copy link
Contributor

@maximilien maximilien 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 doing this.
/test pull-knative-client-integration-tests

@daisy-ycguo
Copy link
Member Author

/test pull-knative-client-integration-tests-latest-release

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2020
@navidshaikh
Copy link
Collaborator

/retest

@daisy-ycguo
Copy link
Member Author

I will rebase it when #1068 gets merged. Let's see if the lint test could pass.

@daisy-ycguo daisy-ycguo force-pushed the refactor-svc branch 2 times, most recently from 3158cdc to 487b022 Compare October 19, 2020 06:58
@daisy-ycguo
Copy link
Member Author

@maximilien @rhuss @navidshaikh @dsimansk I think this one is ready for review. Will you take a look ? Thank you.

Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

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

As per meeting yesterday, this is good to go, right @navidshaikh ?

LGTM

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2020
pkg/kn/flags/podspec.go Outdated Show resolved Hide resolved
@daisy-ycguo daisy-ycguo force-pushed the refactor-svc branch 2 times, most recently from c5ac135 to 753a0f3 Compare October 27, 2020 05:23
Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

lgtm, please remove the commented code and add a CHANGELOG entry, we should be good to merge then.

pkg/kn/flags/podspec.go Outdated Show resolved Hide resolved
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-client-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/kn/commands/service/configuration_edit_flags.go 83.5% 87.5% 4.0
pkg/kn/flags/podspec.go 97.6% 78.4% -19.2
pkg/kn/flags/podspec_helper.go Do not exist 77.9%
pkg/serving/config_changes.go 76.2% 85.0% 8.8
pkg/util/corev1_helper.go Do not exist 70.8%

Copy link
Collaborator

@navidshaikh navidshaikh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, maximilien, navidshaikh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [maximilien,navidshaikh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot merged commit ed67482 into knative:master Oct 27, 2020
dsimansk added a commit to dsimansk/client that referenced this pull request Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants