Merged
Conversation
howardjohn
reviewed
Jan 16, 2020
manifests/UPDATING-CHARTS.md
Outdated
Member
There was a problem hiding this comment.
maybe suggest build_with_container=1 until we get that as the default
Contributor
Author
There was a problem hiding this comment.
i was thinking the make command would include that. this is a target that it's never really possible to do a local build.
howardjohn
reviewed
Jan 16, 2020
Member
howardjohn
left a comment
There was a problem hiding this comment.
I think you picked up extra changes in here. README lgtm
Member
|
I am not sure what you mean? in this repo container build is off by
default. Or are users expected to run these commands from operator/?
We should have all make targets in the top level I think
…On Wed, Jan 15, 2020, 5:48 PM Martin Ostrowski ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In manifests/UPDATING-CHARTS.md
<#20228 (comment)>:
> +If this is an edit to existing charts or values that doesn't change the schema, make your changes in the `manifests`
+directory first.
+
+### Step 2. Make corresponding values changes in [operator/data/profiles/default.yaml](../operator/data/profiles/default.yaml)
+
+The values.yaml in `manifests` are only used for direct Helm based installations, which is being deprecated.
+If any values.yaml changes are being made, the same changes must be made in the `operator/data/profiles/default.yaml`
+file, which must be in sync with the Helm values in `manifests`.
+
+### Step 3. Update the validation schema
+
+Istioctl uses a [schema](../operator/pkg/apis/istio/v1alpha1/values_types.proto) to validate the values. Any changes to
+the schema must be added here, otherwise istioctl users will see errors.
+Once the schema file is updated, run:
+```bash
+$ make gen-operator-proto
i was thinking the make command would include that. this is a target that
it's never really possible to do a local build.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#20228?email_source=notifications&email_token=AAEYGXIJROZ46AUWGKK5IJLQ564HZA5CNFSM4KHMGFB2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCR5WSIA#discussion_r367195733>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXMXJVWD53KKPXUO5V3Q564HZANCNFSM4KHMGFBQ>
.
|
Contributor
Author
|
i mean even if build container is off, we turn it on for this target. we can echo an explanation for the time being so people don't get confused. |
Member
|
we cannot turn it on or off selectively. I guess if you run make commands
from within operator/ it will use the build container but really that's
just because we haven't merged the two makefiles yet?
Id the targets are only in operator readme let's just make it clear you
need to be in operator/
…On Wed, Jan 15, 2020, 9:32 PM Martin Ostrowski ***@***.***> wrote:
i mean even if build container is off, we turn it on for this target. we
can echo an explanation for the time being so people don't get confused.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#20228?email_source=notifications&email_token=AAEYGXI562WDWPKCJXZDKIDQ57WN5A5CNFSM4KHMGFB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJC2TEQ#issuecomment-574990738>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEYGXJ5BDD7UH6HCUX6RP3Q57WN5ANCNFSM4KHMGFBQ>
.
|
0fabd61 to
9888b2c
Compare
howardjohn
approved these changes
Jan 23, 2020
Member
|
Can you rebase? This is blocking devs who don't know how to submit their changes |
Member
|
can we rebase this? |
Member
|
@ostromart any plans to rebase? it would be pretty handy to point folks to this |
f8a83ea to
b4b2534
Compare
Collaborator
|
In response to a cherrypick label: #20228 failed to apply on top of branch "release-1.5": |
ostromart
added a commit
to ostromart/istio
that referenced
this pull request
Feb 7, 2020
istio-testing
pushed a commit
that referenced
this pull request
Feb 8, 2020
sdake
pushed a commit
to sdake/istio
that referenced
this pull request
Feb 21, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a first pass to steer users to everything they need to do when making changes to the manifests dir. Lots of opportunity for process improvement here :-) At least this is a start.
This needs to be rebased once #20218 merges, in the meantime, please ignore everything except the markdown files.