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

Deprecate non-specification field #230

Merged
merged 2 commits into from
Sep 22, 2021
Merged

Deprecate non-specification field #230

merged 2 commits into from
Sep 22, 2021

Conversation

ForestEckhardt
Copy link
Contributor

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

@ryanmoran
Copy link
Member

It may not appear to be used in packit or by our buildpacks, but jam uses this field here. Maybe we can figure out how not to use this in jam and then we can remove it.

@ForestEckhardt
Copy link
Contributor Author

ForestEckhardt commented Sep 21, 2021

Interesting because as far as I can figure from the spec the buildpack table has no SHA field. I'll take a look!

@ryanmoran
Copy link
Member

Its not used to read out a value from the buildpack.toml. Its just used to hold the value between that point in the jam code and the formatter.

@ForestEckhardt
Copy link
Contributor Author

@ryanmoran Here is a PR to remove the use of the Buildpack.SHA256 field from jam paketo-buildpacks/jam#17

@ryanmoran
Copy link
Member

Are you planning to bump the major version of packit when you remove this?

@ForestEckhardt
Copy link
Contributor Author

I hadn't really thought about how I would classify the bump. I think that by definition it would have to be but the field was never parsed it was just part of the structure. It feels a little silly to have a major bump over such a small change.

@ryanmoran
Copy link
Member

Yes. But strictly speaking, its a backwards-incompatible change. Maybe you should leave the field and just mark it as deprecated which is what we have done in the past. Then, when we get to a point where a major version bump feels warranted because of other, more substantial changes, we can remove it.

@ForestEckhardt
Copy link
Contributor Author

I think that is fair. We can still go forward with the Jam change as well.

@ForestEckhardt ForestEckhardt changed the title Remove unnecessary field Deprecate non-specification field Sep 22, 2021
@ForestEckhardt ForestEckhardt merged commit 6b22133 into main Sep 22, 2021
@ForestEckhardt ForestEckhardt deleted the clean-up branch September 22, 2021 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants