8000 Validate the major.minor as the version matching mechanism for customized manifests by houshengbo · Pull Request #320 · knative/operator · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Validate the major.minor as the version matching mechanism for customized manifests #320

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
Merged
merged 1 commit into from
Oct 23, 2020
Merged

Validate the major.minor as the version matching mechanism for customized manifests #320

merged 1 commit into from
Oct 23, 2020

Conversation

houshengbo
Copy link
Contributor
@houshengbo houshengbo commented Oct 23, 2020 •
edited
Loading

Proposed Changes

  • Instead of checking whether versions match in terms of major, minor and patch numbers, we only check the major and minor numbers.

Release Note

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 23, 2020
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 23, 2020
knative-prow-robot
knative-prow-robot reviewed Oct 23, 2020
View reviewed changes
Copy link
Contributor
@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

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

@houshengbo: 2 warnings.

In response to this:

Proposed Changes

  • Instead of checking whether versions match in terms of major, minor and patch numbers, we only check the major and minor numbers.
  • Use {VERSION} instead of ${VERSION} as the annotation to replace with the spec.version, because we have to add an additional \ to make $visible.

Release Note

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.

pkg/reconciler/common/releases_test.go Outdated
SERVING_CORE = "testdata/kodata/knative-serving/" + VERSION + "/serving-core.yaml"
SERVING_HPA = "testdata/kodata/knative-serving/" + VERSION + "/serving-hpa.yaml"
EVENTING_CORE = "testdata/kodata/knative-eventing/" + VERSION + "/eventing-core.yaml"
IN_MEMORY_CHANNEL = "testdata/kodata/knative-eventing/" + VERSION + "/in-memory-channel.yaml"
SERVING_VERSION_CORE = "testdata/kodata/knative-serving/${VERSION}/serving-core.yaml"
SERVING_VERSION_HPA = "testdata/kodata/knative-serving/${VERSION}/serving-hpa.yaml"
SERVING_VERSION_CORE = "testdata/kodata/knative-serving/{VERSION}/serving-core.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SERVING_VERSION_CORE = "testdata/kodata/knative-serving/{VERSION}/serving-core.yaml"
ServingVersionCore = "testdata/kodata/knative-serving/{Version}/serving-core.yaml"

Golint naming: don't use ALL_CAPS in Go names; use CamelCase. More info.

pkg/reconciler/common/releases_test.go Outdated
SERVING_VERSION_CORE = "testdata/kodata/knative-serving/${VERSION}/serving-core.yaml"
SERVING_VERSION_HPA = "testdata/kodata/knative-serving/${VERSION}/serving-hpa.yaml"
SERVING_VERSION_CORE = "testdata/kodata/knative-serving/{VERSION}/serving-core.yaml"
SERVING_VERSION_HPA = "testdata/kodata/knative-serving/{VERSION}/serving-hpa.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SERVING_VERSION_HPA = "testdata/kodata/knative-serving/{VERSION}/serving-hpa.yaml"
ServingVersionHpa = "testdata/kodata/knative-serving/{Version}/serving-hpa.yaml"

Golint naming: don't use ALL_CAPS in Go names; use CamelCase. More info.

antoineco
antoineco approved these changes Oct 23, 2020
View reviewed changes
Copy link
@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

/lgtm

pkg/reconciler/common/releases.go
manifestVersion := u.GetLabels()[key]
if targetVersion != manifestVersion && manifestVersion != "" {
if manifestVersion != "" && semver.MajorMinor(targetVersion) != semver.MajorMinor(manifestVersion) {
Copy link

Choose a reason for hiding this comment

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

Nice! Didn't know MajorMinor()

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2020
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2020
antoineco
antoineco approved these changes Oct 23, 2020
View reviewed changes
Copy link
@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, houshengbo

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 removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2020
@antoineco
Copy link

/lgtm

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

/test pull-knative-operator-upgrade-tests

@knative-prow-robot knative-prow-robot merged commit d7e383e into knative:master Oct 23, 2020
houshengbo pushed a commit to houshengbo/operator-2 that referenced this pull request Oct 23, 2020
Validate the major.minor as the version matching mechanism for custom…
2d96c37 
…ized manifests (knative#320)
houshengbo pushed a commit to houshengbo/operator-2 that referenced this pull request Oct 23, 2020
Validate the major.minor as the version matching mechanism for custom…
3c1f363 
…ized manifests (knative#320)
knative-prow-robot pushed a commit that referenced this pull request Oct 23, 2020
Validate the major.minor as the version matching mechanism for custom…
2c39129 
…ized manifests (#320) (#321)
houshengbo pushed a commit that referenced this pull request Oct 23, 2020
[release-0.17] Validate the major.minor as the version matching mecha…
6d50842 
…nism for customized manifests (#322)

* Validate the major.minor as the version matching mechanism for customized manifests (#320)

* Update the func to check the deployment for test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knative-prow-robot knative-prow-robot knative-prow-robot left review comments

@antoineco antoineco antoineco approved these changes

@Cynocracy Cynocracy Awaiting requested review from Cynocracy

@trshafer trshafer Awaiting requested review from trshafer

Assignees

@antoineco antoineco

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@houshengbo @knative-prow-robot @antoineco
0