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

Fix project.toml build env to match spec #1479

Merged
merged 6 commits into from
Aug 24, 2022
Merged

Conversation

jkutner
Copy link
Member

@jkutner jkutner commented Jul 6, 2022

Summary

Change the parsing of project.toml's io.buildpack.build.env to match the project descriptor spec

https://cloud-native.slack.com/archives/C033DV9EBDF/p1657116095577129

Output

Before

[[io.buildpacks.env.build]]
name = "JAVA_OPTS"
value = "-Xmx300m"

After

[[io.buildpacks.build.env]]
name = "JAVA_OPTS"
value = "-Xmx300m"

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner requested a review from a team as a code owner July 6, 2022 15:34
@github-actions github-actions bot added this to the 0.28.0 milestone Jul 6, 2022
@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Jul 6, 2022
@jromero jromero added the breaking-change Known to be non-backwards compatible label Jul 6, 2022
@jromero
Copy link
Member

jromero commented Jul 6, 2022

Although I agree with this change, this may be considered a breaking change. I'd want to take a deeper look into the reasoning for the change, it's potential impact and mitigation strategies.

@sambhav
Copy link
Member

sambhav commented Aug 3, 2022

As discussed in the WG we cannot make this change directly since this is an end-user breaking change and the current implementation is mimicked by other platforms like kpack as well. If we do change it, we should support both the current and the future version of this key.

@jromero
Copy link
Member

jromero commented Aug 3, 2022

@jkutner Can you update this PR with backward compatibility support?

I would envision that we would warn about the misuse of the invalid namespace and that the support may be removed at any point in the future.

@samj1912 was there a timeframe for support of the invalid key?

@sambhav
Copy link
Member

sambhav commented Aug 3, 2022

@jromero I would prefer if we soften that warning. Since this is an end user change that we inadvertently implemented in both pack and kpack I would prefer we keep this key around at least until project descriptor 0.3.

It would be a disservice if we broke users without warning between different versions of pack without a proper migration plan. If we do plan on emitting a deprecation warning, we should also include the pack version where it will be deprecated. I do think however we are in a tricky situation because we implemented a version of the project descriptor 0.2 in the state that it was in Summer 2021 and this spec change was not made until March 2022. In the future I believe we should have a strict policy about not implementing spec related changes before they are released.

…ct.toml

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@jkutner jkutner force-pushed the jkutner/fix-proj-build-env branch from 95ece35 to 71021c7 Compare August 12, 2022 18:25
@codecov
Copy link

codecov bot commented Aug 12, 2022

Codecov Report

Merging #1479 (71021c7) into main (6575218) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head 71021c7 differs from pull request most recent head 1c1eb83. Consider uploading reports for the commit 1c1eb83 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1479      +/-   ##
==========================================
- Coverage   81.29%   81.26%   -0.03%     
==========================================
  Files         154      154              
  Lines       10014    10018       +4     
==========================================
  Hits         8140     8140              
- Misses       1393     1397       +4     
  Partials      481      481              
Flag Coverage Δ
os_linux 80.01% <0.00%> (-0.01%) ⬇️
os_macos 77.44% <0.00%> (-0.06%) ⬇️
os_windows 81.14% <0.00%> (-0.03%) ⬇️
unit 81.26% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jkutner
Copy link
Member Author

jkutner commented Aug 12, 2022

@jromero @samj1912 i've updated this to include backwards compat

Signed-off-by: Joe Kutner <jpkutner@gmail.com>
@sambhav sambhav enabled auto-merge August 12, 2022 18:55
@jkutner
Copy link
Member Author

jkutner commented Aug 24, 2022

@jromero can you give this a look-see?

Copy link
Member

@jromero jromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Known to be non-backwards compatible type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants