Improve channel updates #7133
Improve channel updates #7133
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Cleanup of #6829 |
|
/assign @justinsb |
| } | ||
|
|
||
| name := b.cluster.ObjectMeta.Name + "-addons-bootstrap" | ||
| // manifestURL, err := a.GetManifestFullUrl() |
There was a problem hiding this comment.
Any reason to leave the commented out section here?
There was a problem hiding this comment.
Nope, good catch. I cleaned that up.
| manifestHash, err := utils.HashString(&rawManifest) | ||
| klog.V(4).Infof("hash %s", manifestHash) | ||
| if err != nil { | ||
| manifestHash = "" |
There was a problem hiding this comment.
Typically a good idea to warn if there's any error we're ignoring. I'd probably return error here, as we should always be able to hash a string!
| // HashString Takes String and returns a sha1 hash represented as a string | ||
| func HashString(s *string) (string, error) { | ||
| h := sha1.New() | ||
| ts := strings.TrimSpace(*s) |
There was a problem hiding this comment.
Trimming the string probably belongs in the caller, not a generic HashString function
| ) | ||
|
|
||
| // HashString Takes String and returns a sha1 hash represented as a string | ||
| func HashString(s *string) (string, error) { |
There was a problem hiding this comment.
We probably shouldn't take a string pointer - it's a ref type anyway, and we can't pass nil
| if err != nil { | ||
| return "", err | ||
| } | ||
| sha := h.Sum(nil) // "sha" is uint8 type, encoded in base16 |
There was a problem hiding this comment.
Nit: I think the type is [20]byte, actually
|
Looks great - thanks @granular-ryanbonham ! /approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: granular-ryanbonham, justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
I'm going to create a target to fix the golden output, as now we have hashes the golden output is going to change even more often! |
|
Needs a rebase and a run of the new streamlined script in #7291 (It's a one-liner, so you can also just run it!) |
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
justinsb
force-pushed
the
squash_improve_channel_updates
branch
from
48773a1 to
438a651
Compare
The update-hash script didn't update gobindata; will fix that separately.
justinsb
force-pushed
the
squash_improve_channel_updates
branch
from
438a651 to
cd0075d
Compare
justinsb
added
the
lgtm
…ease-1.14 Cherry Pick #7133 to release 1.14
Improve channel updates to use hash of manifests to ensure we always update.