Updates 'grovetccfg' to to write a grove.cfg from the grove servers profile #2606
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
jrushford
force-pushed
the
cdn-2099
branch
2 times, most recently
from
c0b9154 to
4243453
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
dangogh
added
grove
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
Go local variables should be camelCase not snake_case
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
This is unnecessary, Go zero-initializes all variables, so err is already nil.
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
A switch would be cleaner here.
switch name {
case "rfc_compliant":
cfg.RFCCompliant, err = strconv.ParseBool(value)
case "remap_rules_file":
cfg.RemapRulesFile = value
...
}
return err
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
bkup_dir should be camel case, and while Go favors brevity bkup is a bit extreme, I'd recommend backupDir
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
If host == nil, this gets passed to createRulesOldAPI which will panic. This should check if host == nil, and print a good error message and os.Exit(1).
That will also reduce indentation, and reduce variable pre-declarations too, which will make this block much nicer.
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
update_required should be updateRequired
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
The config file is duplicated in GroveConfigPath. Can you change the variables to
const GroveConfigFile = "grove.cfg"
const GroveConfigPath = "/etc/grove/" + GroveConfigFile
To get rid of the magic string?
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
All these strings duplicating the JSON tags aren't great, but unfortunately, I don't think Go has a better way to do this
grove/grovetccfg/grovetccfg.go
Outdated
There was a problem hiding this comment.
Would you mind moving this to traffic_control/lib/go-tc/enum.go? Profile types should be there, so other apps like the TO client can use them.
|
Ok, I commented on all the formatting things I saw. I didn't do an actual review, because that will take longer, and I'll need to install and run it against a TO instance with the expected profile. I'll try to do that as soon as I can find the time. Feel free to fix the things noted now, or wait until the full review, whatever you prefer. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Hey Rob,
I’ve made the requested changes except for this one, I do not see that there is currently any profile enum in go-tc. If necessary, I could add one I suppose.
I had several commits so, I’ve also squashed them.
If it helps, on ipcdn-cache-59.cdnlab.comcast.net, I have a cdn-in-a-box running with the current database from staging where I have added the GROVE_PROFILE to profile_type_values and have a EDGE1_XCR_GROVE_0.1-9001 built. I’ve been running this script on cache-59 for testing, you could use my hostname below or pick another and change it’s profile to use this one:
#!/bin/bash
./grovetccfg -ignore-update-flag -host cdn-ec-sfb-001-01 -touser jrushford -topass twelve -insecure -tourl https://localhost:6443 -pretty
|
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
the current server and generate a new 'grove.cfg' file if there are changes.
|
Refer to this link for build results (access rights to CI server needed): |
|
@rob05c How’s the review of this coming along? |
|
@dewrich, @dangogh, @DylanVolz can i get a review of this pr. It is pretty straight forward. It writes a grove.cfg file for a server when a grove profile is used. |
grove profile information.
|
Refer to this link for build results (access rights to CI server needed): |
5 participants
Modified grovetccfg to read a grove server profile from the traffic_ops and write a grove.cfg from that profile id there have been changes. Fixes #2607 and depends on #2517 and #2523. Both #2517 and #2523 are merged onto master.