You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains 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
Moved logic to compute necessary object to get hash of k8s object to utils.
Added support to generate yaml of any k8s object.
The above changes will be used to get diff of k8s object in operator whenever hash of object changes to have better visibility around the changes that triggers restart of k8 clusters.
I don't want to nitpick, but I think that the hashing logic belongs to the hashing module, not the utils. If the problem is calling the hashing function in one of the yaml getters, I suggest you move them to the hashing module, too. They are going to be called in the contest of computing hashes anyway.
What about adding the logic to show the diff of two objects here as well? I am sure that other operators would benefit from that too. Or do you think that it would be too Flink specific?
Regarding the logic of hashing function. The job of hashing module according to me is to generate the hash of the object. IMO how it gets that relevant object can be put into utils so that other modules if they want to use can use the same logic.
Yeah I will add someone from CI
Good suggestion, we can actually put that logic here as it is not specific to Flink object.
I don't want to nitpick, but I think that the hashing logic belongs to the hashing module, not the utils. If the problem is calling the hashing function in one of the yaml getters, I suggest you move them to the hashing module, too. They are going to be called in the contest of computing hashes anyway.
I strongly support this. The reason being that the function GetHashObjectOfKubernetes takes some very specific parts of an object which matter for hashing only, while function GetYamlOfHashObjectOfK8sObject has a dependency on this particular behaviour. I think we probably do not need GetYamlOfObject, so that leaves nothing to utils module.
* Adding support to get Yaml for k8s object
* Added logic to compute yaml diff, refactored hashing helper
* Nit changes for review
* Updating link for unified context
mks-m
pushed a commit
that referenced
this pull request
May 3, 2020
* Adding support to get Yaml for k8s object
* Added logic to compute yaml diff, refactored hashing helper
* Nit changes for review
* Updating link for unified context
mks-m
pushed a commit
that referenced
this pull request
May 4, 2020
* Adding support to get Yaml for k8s object
* Added logic to compute yaml diff, refactored hashing helper
* Nit changes for review
* Updating link for unified context
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.
AshishKhatkar
requested review from
chlgit,
mattmb and
Baisang
and removed request for
mattmb and
Baisang
Changes done :