[GitHub] [incubator-yunikorn-core] wilfred-s commented on a change in pull request #207: [YUNIKORN-405] Added checksum validation

[GitBox <gi...@apache.org>]

wilfred-s commented on a change in pull request #207:
URL: https://github.com/apache/incubator-yunikorn-core/pull/207#discussion_r503695315



##########
File path: pkg/common/configs/config.go
##########
@@ -35,7 +35,7 @@ import (
 // set of scheduler resources.
 type SchedulerConfig struct {
 	Partitions []PartitionConfig
-	Checksum   [32]byte `yaml:"-" json:"-"`
+	Checksum   [32]byte `yaml:",omitempty" json:",omitempty"`

Review comment:
       If we marshal the checksum in the config there should be no need to handle them separately in the code.
   This seems to be a double up with handling in the code

##########
File path: pkg/webservice/handlers.go
##########
@@ -378,6 +385,20 @@ func updateConfig(w http.ResponseWriter, r *http.Request) {
 	buildUpdateResponse(true, "", w)
 }
 
+func getConfigurationString(requestBytes []byte, checksum [32]byte) string {
+	newConf := string(requestBytes)
+	checksumString := fmt.Sprintf("%v", checksum)
+	checksumString = strings.ReplaceAll(checksumString, " ", ",")
+	checksumString = "checksum: " + checksumString

Review comment:
       In the `getClusterConfig()` we use the following string to write the checksum:
   `sha256 := fmt.Sprintf("\nsha256 checksum: %X", conf.Checksum)`
   The `%X` converts the checksum into a hex string which is the same for both json and yaml. 
   
   I cannot assess if this works for both or how this relates to the current layout we have in the get call.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org