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

Do not check DeprecationDate when comparing dependencies #240

Merged
merged 3 commits into from
May 26, 2023

Conversation

c0d1ngm0nk3y
Copy link
Contributor

Summary

This might be an alternative to #233 in addressing short comings when comes to comparing the metadata of a dependency.

  • DeprecationDate does not work with reflect.deepEqual for 2 reasons
    • After Marshal and Unmarshal, the time.Time cannot be compared anymore, since it will loose some precision because of the missing UnmarshalTOML
    • Even WITH the fixed unmarshaling, the compare will work, but the reflect.deepEqual will still fail because it compares the exact internal structure.
  • If the CPEs array is empty, those a arrays are not considered to be reflect.deepEqual

Use Cases

As discussed in #233, comparing the metadata of a dependency is probably not needed at all if sha256 is the same. But at least the comparison should be successful in those cases since the dependency caching currently depends on it.

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).

@c0d1ngm0nk3y c0d1ngm0nk3y requested a review from a team as a code owner May 12, 2023 14:03
@dmikusa dmikusa added type:bug A general bug semver:patch A change requiring a patch version bump labels May 18, 2023
buildpack.go Outdated Show resolved Hide resolved
c0d1ngm0nk3y and others added 2 commits May 26, 2023 10:51
Co-authored-by: Daniel Mikusa <dan@mikusa.com>
@modulo11
Copy link
Contributor

This looks like a cost-efficient and compatible alternative until #233 can be continued on a v2 branch. @dmikusa is there anything holding us back from merging it?

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:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants