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.
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
Save Jobs History on Flink #6
Save Jobs History on Flink #6
Changes from 2 commits
b47cc34
3f1baa8
07cd6ab
4cc9b4d
15559f5
7be14c5
1fc95d9
a3b0bb8
dcbbd35
fbc07a3
bc1a2ea
cd672be
dbbf520
c92ddc4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that this has 'historyserver' in the name, so it gets used specifically just for this and not much more :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PersistentVolume is also not namespaced, and should get same treatment as
StorageClass
with.Release.Name
. sorry for not catching that earlier.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ranchodeluxe I think this still needs to be fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying both here is redundant. Also, I don't actually see a service account named flink be defined here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for schema items that we mark as required, we should not specify them here. That way, when the user tries to deploy this without those set, the schema setup will kick in and complain - I think specifying empty values here negates that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minLength
invalues.schema.json
handles validation for this and two of them were unneeded given changes that weren't in this branchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think undoing the 'required' is much cleaner than enforcing them via
minLength: 1
. If there is a future change coming in that makes these required, we can modify the schema at that point, no?