Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using regex to update dependencies in builder.toml, package.toml, and buildpack.toml #90

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

dmikusa
Copy link
Contributor

@dmikusa dmikusa commented Oct 1, 2021

This includes:

  • metadata dependencies
  • composite buildpack.toml order groups
  • package.toml files
  • builder.toml files

Instead of using complicated, fragile regular expressions the code unmarshals the file from TOML to a map, traverses the map, locates the items to update, if found updates them, and then writes out the map as TOML. Using maps is a pain here, but it's done because unmarshaling to structs would require a lot of different, complicated structs that don't already exist. This would be a lot of effort because we only care about a very small subset of these files, creating structs to map out a bunch of stuff we don't care about.

Great care was put into ensuring that we are checking for keys to exist and for type assertions to be true. This should return sensible errors or skip bad data where possible.

The one downside of this approach that should be noted is that the TOML generated when we marshal the map back out to a file is controlled 100% by the TOML library. Thus the order for these files will likely change after the first time a dependency is updated. Fortunately, the order defined by the TOML library is static, so it won't change from update-to-update. There are just no options to control how it writes the TOML, so it's not possible to match our existing format.

On the plus side, this makes the update process way more robust because it also does not care about the incoming TOML format either. Any valid TOML format will work, which was not the case with the previous regular expression based update approach. That required a very specific TOML format.

Signed-off-by: Daniel Mikusa dmikusa@vmware.com

Summary

Use Cases

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@dmikusa dmikusa added type:task A general task semver:patch A change requiring a patch version bump labels Oct 1, 2021
@dmikusa dmikusa requested a review from a team October 1, 2021 12:46
…, and buildpack.toml

This includes:
- metadata dependencies
- composite buildpack.toml order groups
- package.toml files
- builder.toml files

Instead of using complicated, fragile regular expressions the code unmarshals the file from TOML to a map, traverses the map, locates the items to update, if found updates them, and then writes out the map as TOML. Using maps is a pain here, but it's done because unmarshaling to structs would require a lot of different, complicated structs that don't already exist. This would be a lot of effort because we only care about a very small subset of these files, creating structs to map out a bunch of stuff we don't care about.

Great care was put into ensuring that we are checking for keys to exist and for type assertions to be true. This should return sensible errors or skip bad data where possible.

The one downside of this approach that should be noted is that the TOML generated when we marshal the map back out to a file is controlled 100% by the TOML library. Thus the order for these files will likely change after the first time a dependency is updated. Fortunately, the order defined by the TOML library is static, so it won't change from update-to-update. There are just no options to control how it writes the TOML, so it's not possible to match our existing format. This includes erasing comments. The code specifically handles leading comments, which are often license headers, to ensure that they are preserved. Other comments will be removed though.

On the plus side, this makes the update process way more robust because it also does not care about the incoming TOML format either. Any valid TOML format will work, which was not the case with the previous regular expression based update approach. That required a very specific TOML format. In addition, the TOML format output by the library is less compact & this should reduce the frequency of merge conflicts when updating dependencies of buildpacks.

Signed-off-by: Daniel Mikusa <dmikusa@vmware.com>
…an additional space is added

Signed-off-by: Daniel Mikusa <dmikusa@vmware.com>
@dmikusa dmikusa merged commit eb2c8cb into main Oct 4, 2021
@dmikusa dmikusa deleted the update-updaters branch October 4, 2021 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant