More flexible fs.ChecksumCalculator #167

andymoe · 2021-04-30T16:07:34Z

Summary

Made the following additions to the API in order to allow for adding additional data to the hash after a set of files or directories have been processes. The new API is similar to the Hash interface however other function names may be more clear.

calculator := fs.NewChecksumCalculator()

calculator.WritePaths("/my/file", "/my/folder")

// currently Write mirrors Hash.Write signature but we probably don't need the byte length returned
calculator.Write([]bytes{"some string"})
calculator.Write([]bytes{"another string"})

sum := calculator.SumAsHexString()

The old API is still around for backwards compatibility:

calculator := fs.NewChecksumCalculator()
sum := calculator.Sum("/my/file", "/my/folder")

Use Case

func ShouldRun() (bool, string, error) {
  npm := pexec.NewExecutable("npm")

  buffer := bytes.NewBuffer(nil)
  npm.Execute(pexec.Execution{
      Args:   []string{"config", "get", "node-version"},
      Dir:    "",
      Stdout: buffer,
      Stderr: buffer,
  }))

  calculator := fs.NewChecksumCalculator()
  err := calculator.WritePaths("node_modules")
  if err != nil {
    return false, "", fmt.Errorf("unable to get sha of node_modules for layer caching: %w", err)
  }
  _, err = calculator.Write(buffer.Bytes())
    if err != nil {
    return false, "", fmt.Errorf("unable to determine node version for layer caching: %w", err)
  }

  sum := calculator.SumAsHexString()
  
  cacheSha, ok := metadata["cache_sha].(string)
    if !ok || sum != cacheSha {
    return true, sum, nil
  }
  
  return false, "", nil
}

With no API changes (still viable option I guess)

func ShouldRun() (bool, string, error) {
  npm := pexec.NewExecutable("npm")
  
  buffer := bytes.NewBuffer(nil)
  npm.Execute(pexec.Execution{
      Args:   []string{"config", "get", "node-version"},
      Dir:    "",
      Stdout: buffer,
      Stderr: buffer,
  }))

  calculator := fs.NewChecksumCalculator()
  sum, err := calculator.sum("node_modules")
  if err != nil {
     return false, "", fmt.Errorf("unable to get sha of node_modules for layer caching: %w", err)
  }

  hash := sha256.New()
  _, err = hash.Write(buffer.Bytes())
  if err != nil {
     return false, "", fmt.Errorf("unable to get sha of node_modules for layer caching: %w", err)
  }

  _, err = hash.Write([]byte(sum))
    if err != nil {
    return false, "", fmt.Errorf("unable to get sha of node_modules for layer caching: %w", err)
  }

  sum = hex.EncodeToString(hash.sum(nil))

  cacheSha, ok := metadata["cache_sha].(string)
  if !ok || sum != cacheSha {
    return true, sum, nil
  }
  
  return false, "", nil
}

Potential alternative method names

calculator := fs.NewChecksumCalculator()

calculator.AddPaths("/my/file", "/my/folder")
calculator.Add([]bytes{"some string"})
calculator.Add([]bytes{"another string"})

sum := calculator.ResultAsHexString()

ryanmoran

I'd like to understand the intention of an API like this. How would it be used? What use-cases is it solving for?

fs/checksum_calculator.go

ryanmoran · 2021-05-03T23:38:07Z

fs/checksum_calculator.go

+	n, err := c.hash.Write(p)
+	if err != nil {
+		return n, err
+	}
+	return n, err


Suggested change

n, err := c.hash.Write(p)

if err != nil {

return n, err

}

return n, err

return c.hash.Write(p)

Unless you intend to do something different in the error case, these are logically equivalent.

Thanks! I'm debating if it makes sense to hide n entirely from the return is all.

Introduces the following methods: - WriteFiles(files ...string) (error) - Write(p []byte) (int, error) - SumAsHexString() Sum(files ...string) (string, error) remains for backward compatability.

andymoe · 2021-05-04T00:32:26Z

If or when we decide something like this API is OK I should backfill tests for the public methods. Right now the new additions are tested indirectly because Sum uses them all.

ryanmoran · 2021-06-09T17:17:47Z

I think it could make sense to expose some of these lower-level APIs to the buildpack author, but I think it may make more sense as a new type that is distinct from the fs.ChecksumCalculator. Specifically, I think the proposed changes break an implicit contract which is that the fs.ChecksumCalculator is stateless. The new API moves state into the calculator and thus makes it difficult to reuse or use in cases of parallel operation. I'd like to propose a different API that would create a new stateful type that can be reused by the fs.ChecksumCalculator and therefore keep this implicit stateless contract in-place.

type Checksum struct {
  hash hash.Hash
}

func (c Checksum) WritePaths(paths ...string) error {
  // recursively hashes a filepath, keeping state in c.hash
}

func (c Checksum) Write(b []byte) (int, error) {
  // writes b into c.hash
}

func (c Checksum) EncodeToString() string {
  // returns a hex string encoding of the hash value
}

andymoe · 2021-06-09T17:27:26Z

Sounds good. I’ll take a crack at it.

…

On Wed, Jun 9, 2021 at 10:18 AM Ryan Moran ***@***.***> wrote: I think it could make sense to expose some of these lower-level APIs to the buildpack author, but I think it may make more sense as a new type that is distinct from the fs.ChecksumCalculator. Specifically, I think the proposed changes break an implicit contract which is that the fs.ChecksumCalculator is stateless. The new API moves state into the calculator and thus makes it difficult to reuse or use in cases of parallel operation. I'd like to propose a different API that would create a new stateful type that can be reused by the fs.ChecksumCalculator and therefore keep this implicit stateless contract in-place. type Checksum struct { hash hash.Hash } func (c Checksum) WritePaths(paths ...string) error { // recursively hashes a filepath, keeping state in c.hash } func (c Checksum) Write(b []byte) (int, error) { // writes b into c.hash } func (c Checksum) EncodeToString() string { // returns a hex string encoding of the hash value } — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#167 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACTA3LPQ4NOYXUO5JIWULTR6O4XANCNFSM434SNUMA> .

andymoe · 2021-06-09T17:29:31Z

Shall I close this PR in favor of a separate one or reuse this one?

…

On Wed, Jun 9, 2021 at 10:27 AM Andy Brown ***@***.***> wrote: Sounds good. I’ll take a crack at it. On Wed, Jun 9, 2021 at 10:18 AM Ryan Moran ***@***.***> wrote: > I think it could make sense to expose some of these lower-level APIs to > the buildpack author, but I think it may make more sense as a new type that > is distinct from the fs.ChecksumCalculator. Specifically, I think the > proposed changes break an implicit contract which is that the > fs.ChecksumCalculator is stateless. The new API moves state into the > calculator and thus makes it difficult to reuse or use in cases of parallel > operation. I'd like to propose a different API that would create a new > stateful type that can be reused by the fs.ChecksumCalculator and > therefore keep this implicit stateless contract in-place. > > type Checksum struct { > hash hash.Hash > } > func (c Checksum) WritePaths(paths ...string) error { > // recursively hashes a filepath, keeping state in c.hash > } > func (c Checksum) Write(b []byte) (int, error) { > // writes b into c.hash > } > func (c Checksum) EncodeToString() string { > // returns a hex string encoding of the hash value > } > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#167 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAACTA3LPQ4NOYXUO5JIWULTR6O4XANCNFSM434SNUMA> > . >

ForestEckhardt · 2021-06-09T19:04:30Z

@andymoe feels like six of one, half a dozen of the other to me. If you feel that a new PR is necessary I would not be offended.

ForestEckhardt · 2021-06-25T15:34:36Z

@andymoe bump

sophiewigmore · 2021-08-20T14:50:11Z

Drafting for now due to inactivity

fg-j · 2021-12-01T18:39:35Z

Closing this for now since it's been inactive for a few months. Feel free to reopen if you still want to contribute this work @andymoe

andymoe · 2021-12-01T22:27:20Z

😭 I’m sorry.

…

On Wed, Dec 1, 2021 at 10:39 AM Frankie G-J ***@***.***> wrote: Closing this for now since it's been inactive for a few months. Feel free to reopen if you still want to contribute this work @andymoe <https://github.com/andymoe> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#167 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACTA2RTM5263U53ROTECTUOZTXDANCNFSM434SNUMA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

ForestEckhardt · 2021-12-02T15:33:07Z

My goal was to review and pick this up next. I'll try and do that later this week!

ryanmoran suggested changes May 3, 2021

View reviewed changes

fs/checksum_calculator.go Outdated Show resolved Hide resolved

fs/checksum_calculator.go Outdated Show resolved Hide resolved

andymoe force-pushed the enhance-checksum-calculator branch from 028a7ba to e80baa4 Compare May 3, 2021 23:33

andymoe changed the title ~~WIP: More flexible ChecksumCalculator~~ More flexible fs.ChecksumCalculator May 3, 2021

ryanmoran reviewed May 3, 2021

View reviewed changes

More flexible fs.ChecksumCalculator

50f4366

Introduces the following methods: - WriteFiles(files ...string) (error) - Write(p []byte) (int, error) - SumAsHexString() Sum(files ...string) (string, error) remains for backward compatability.

andymoe force-pushed the enhance-checksum-calculator branch from e80baa4 to 50f4366 Compare May 4, 2021 00:13

andymoe marked this pull request as ready for review May 4, 2021 00:16

andymoe requested a review from a team as a code owner May 4, 2021 00:16

andymoe requested review from ForestEckhardt and paketo-bot-reviewer May 4, 2021 00:16

Merge branch 'main' into enhance-checksum-calculator

4f377e1

sophiewigmore marked this pull request as draft August 20, 2021 14:50

fg-j closed this Dec 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More flexible fs.ChecksumCalculator #167

More flexible fs.ChecksumCalculator #167

andymoe commented Apr 30, 2021 •

edited

Loading

ryanmoran left a comment

ryanmoran May 3, 2021

andymoe May 4, 2021 •

edited

Loading

andymoe commented May 4, 2021 •

edited

Loading

ryanmoran commented Jun 9, 2021

andymoe commented Jun 9, 2021 via email

andymoe commented Jun 9, 2021 via email

ForestEckhardt commented Jun 9, 2021

ForestEckhardt commented Jun 25, 2021

sophiewigmore commented Aug 20, 2021

fg-j commented Dec 1, 2021

andymoe commented Dec 1, 2021 via email

ForestEckhardt commented Dec 2, 2021

More flexible fs.ChecksumCalculator #167

More flexible fs.ChecksumCalculator #167

Conversation

andymoe commented Apr 30, 2021 • edited Loading

Summary

Use Case

With no API changes (still viable option I guess)

Potential alternative method names

ryanmoran left a comment

Choose a reason for hiding this comment

ryanmoran May 3, 2021

Choose a reason for hiding this comment

andymoe May 4, 2021 • edited Loading

Choose a reason for hiding this comment

andymoe commented May 4, 2021 • edited Loading

ryanmoran commented Jun 9, 2021

andymoe commented Jun 9, 2021 via email

andymoe commented Jun 9, 2021 via email

ForestEckhardt commented Jun 9, 2021

ForestEckhardt commented Jun 25, 2021

sophiewigmore commented Aug 20, 2021

fg-j commented Dec 1, 2021

andymoe commented Dec 1, 2021 via email

ForestEckhardt commented Dec 2, 2021

andymoe commented Apr 30, 2021 •

edited

Loading

andymoe May 4, 2021 •

edited

Loading

andymoe commented May 4, 2021 •

edited

Loading