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

Validate scale and container concurrency options when updating configuration resource #279

Merged
merged 12 commits into from
Jul 25, 2019

Conversation

toVersus
Copy link
Contributor

@toVersus toVersus commented Jul 19, 2019

I would like to do more contributions on knative/client but currently some large PRs are under reviewing. Therefore, I decided to read and refactor current code bases without bothering core contributors. If it's too much troubles, just let me know!

Proposed Changes

  • Validate the scale and container concurrency options and returns error when invalid value is found.
  • Add unit/e2e tests for handling invalid user inputs
  • Use GetContainer method for extracting container spec from revision template
  • Use constant values for annotation keys
  • Add missing unit test for TestUpdateContainerImage method

Release Note

None

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 19, 2019
@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2019
@knative-prow-robot
Copy link
Contributor

Hi @toVersus. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 19, 2019
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.

@toVersus Thanks for contribution. Yes, we have some pending PRs on core and things should clear up next week. Also, PR #274 will require some changes for your e2e but should not be too hard and clean things a bit even more. Leaving some initial review. Cheers 🍻

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2019
@knative-prow-robot knative-prow-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2019
@sixolet
Copy link
Contributor

sixolet commented Jul 22, 2019

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2019
@toVersus toVersus force-pushed the fix/validate-options branch from ab52a32 to d81391d Compare July 23, 2019 01:23
@rhuss
Copy link
Contributor

rhuss commented Jul 24, 2019

@toVersus have you rebased to the latest master commits ? If not, please do as this would fix the integration test error.

@toVersus toVersus force-pushed the fix/validate-options branch from 755e982 to 010db91 Compare July 24, 2019 10:34
@toVersus
Copy link
Contributor Author

@rhuss Hmm, I have already rebased to the latest master commit, but it seems something wrong with the tests against the latest release job. I rebased to the previous commit and triggered the CI job again.

@toVersus
Copy link
Contributor Author

@rhuss It looks like an error comes from the flakiness of the test. Now, it's green!
I think this PR is ready for the review because #274 is already merged.

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.

minor typo in error message and few suggestions about using the flag-name in error message than using the annotation key

pkg/serving/config_changes.go Outdated Show resolved Hide resolved
pkg/serving/config_changes.go Outdated Show resolved Hide resolved
pkg/serving/config_changes.go Outdated Show resolved Hide resolved
pkg/serving/v1alpha1/client_test.go Outdated Show resolved Hide resolved
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
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 78.6% 77.5% -1.1
pkg/serving/config_changes.go 65.5% 75.8% 10.2
pkg/serving/revision_template.go 80.0% 87.5% 7.5

@toVersus
Copy link
Contributor Author

@navidshaikh Thanks for reviewing! I addressed your comments.

Copy link
Contributor

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thanks a lot ! lgtm

/approve
/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhuss, toVersus

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2019
@knative-prow-robot knative-prow-robot merged commit b7808b0 into knative:master Jul 25, 2019
@toVersus toVersus deleted the fix/validate-options branch July 28, 2019 06:09
navidshaikh added a commit to navidshaikh/client that referenced this pull request Mar 2, 2020
Update version in label and tag for kn version
navidshaikh added a commit to navidshaikh/client that referenced this pull request Mar 2, 2020
Update version in label and tag for kn version
coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020
Part of knative/serving#2555.

Bonus: block running E2E tests using knative-nightly GCR as well.
dsimansk added a commit to dsimansk/client that referenced this pull request Oct 10, 2023
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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants