Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

update controller version. #306

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

update controller version. #306

wants to merge 4 commits into from

Conversation

morvencao
Copy link
Member

@morvencao morvencao commented Sep 29, 2019 •
edited
Loading

update controller version.

@morvencao morvencao requested a review from a team as a code owner September 29, 2019 05:58
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Sep 29, 2019
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Sep 29, 2019
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 29, 2019
ostromart
ostromart reviewed Sep 30, 2019
View reviewed changes
cmd/mesh/manifest-migrate.go Outdated
@@ -84,7 +84,7 @@ func migrateFromFiles(rootArgs *rootArgs, args []string, l *logger) {

// translateFunc translates the input values and output the result
func translateFunc(values []byte, l *logger) {
ts, err := translate.NewReverseTranslator(version.NewMinorVersion(1, 3))
ts, err := translate.NewReverseTranslator(version.NewMinorVersion(1, 4))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one of the places where hard coding should be taken out and ./version package used.

cmd/mesh/manifest-versions.go Outdated
@@ -28,7 +28,7 @@ import (
)

const (
versionsMapURL = "https://raw.githubusercontent.com/istio/operator/master/version/versions.yaml"
versionsMapURL = "https://raw.githubusercontent.com/morvencao/operator/br_update_versions_map/version/versions.yaml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please mark this with DONOTMERGE or something so it doesn't sneak it?

Copy link
Member

Choose a reason for hiding this comment

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

since #305 is merged, @morvencao can we get this updated so this PR can be merged?

pkg/translate/translate_test.go Outdated
@@ -24,7 +24,7 @@ import (
"istio.io/operator/pkg/version"
)

func TestProtoToValuesV13(t *testing.T) {
func TestProtoToValuesV14(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We really want a new function for V14 - for now, feel free to have it just call V13.

pkg/translate/translate_value.go Outdated
@@ -46,7 +46,7 @@ type ReverseTranslator struct {
var (
// ReverseTranslators maps a minor version to a corresponding ReverseTranslator.
ReverseTranslators = map[version.MinorVersion]*ReverseTranslator{
version.NewMinorVersion(1, 3): {
version.NewMinorVersion(1, 4): {
Copy link
Contributor

Choose a reason for hiding this comment

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

should use ./version

pkg/translate/translate_value_test.go Outdated
@@ -357,7 +357,7 @@ autoInjection:
`,
},
}
tr, err := NewReverseTranslator(version.NewMinorVersion(1, 3))
tr, err := NewReverseTranslator(version.NewMinorVersion(1, 4))
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

version/version.go Outdated
)

var (
// SupportedVersions is a list of chart versions supported by this version of the operator.
// It must be synced with the versions.yaml file.
SupportedVersions = []string{
"1.3.0",
"1.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Also 1.3.1. Actually this is one of the places that the script that creates a new branch should update.

version/versions.yaml Outdated
@@ -1,4 +1,7 @@
- operatorVersion: 1.3.0
supportedIstioVersions: 1.3.0
recommendedIstioVersions: 1.3.0
- operatorVersion: 1.4.0
supportedIstioVersions: ">=1.3.0,<=1.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

s/<=1.4/1.4.0/ per other PR.
also could you please add 1.3.1

@morvencao
Copy link
Member Author

/hold

@linsun
Copy link
Member

linsun commented Oct 9, 2019

@morvencao can you address @ostromart's comment and update the version.yaml reference so we can get this merged?

I'm hoping folks can start test controller (on server side) after this.

@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 9, 2019
@morvencao morvencao changed the title [WIP]update version. update version. Oct 9, 2019
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Oct 9, 2019
@sdake
Copy link
Member

sdake commented Oct 22, 2019

@morvencao is this PR still valid? If so, can you rebase, if not can you abandon?

Cheers
-steve

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 22, 2019
@istio-testing istio-testing added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 22, 2019
@morvencao
Copy link
Member Author

morvencao commented Oct 22, 2019 •
edited
Loading

I minimized the change to only update the controller version, other changes has been in the master branch, PTAL @sdake

This PR is not mandatory for 1.4 release, we can merge this after 1.4 is released.

@istio-testing
Copy link

istio-testing commented Oct 22, 2019 •
edited
Loading

@morvencao: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
e2e_operator 034ef25 link /test e2e_operator

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. I understand the commands that are listed here.

@morvencao morvencao changed the title update version. update controller version. Oct 22, 2019
@istio-policy-bot
Copy link

🧭 This issue or pull request has been automatically marked as stale because it has not had activity from an Istio team member since 2019-10-22. It will be closed on 2019-12-06 unless an Istio team member takes action. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ostromart ostromart ostromart left review comments

@linsun linsun linsun left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@morvencao @linsun @sdake @istio-testing @istio-policy-bot @ostromart @googlebot