Support correct specification of project descriptor #1027

jjbustamante · 2022-09-01T21:41:17Z

Problem

The CNB project descriptor v0.2 was updated and the way the environment variables are set was changed. We need to add support for this.

Solution

A new structured was added for parsing the new way of setting environment variables in the project descriptor, but we also kept the previous one for backward compatibility.

Fixes #1017

Signed-off-by: Juan Bustamante jbustamante@vmware.com

codecov-commenter · 2022-09-01T21:47:13Z

Codecov Report

Merging #1027 (a4d08d1) into main (411ffa0) will increase coverage by 0.38%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##             main    #1027      +/-   ##
==========================================
+ Coverage   70.14%   70.53%   +0.38%     
==========================================
  Files         122      122              
  Lines        5658     5726      +68     
==========================================
+ Hits         3969     4039      +70     
  Misses       1313     1313              
+ Partials      376      374       -2

Impacted Files	Coverage Δ
pkg/cnb/env_vars.go	`86.36% <93.93%> (+22.72%)`	⬆️
pkg/cnb/project_descriptor.go	`88.75% <100.00%> (+7.32%)`	⬆️
pkg/reconciler/network_error_reconciler.go	`88.88% <0.00%> (ø)`
pkg/apis/build/v1alpha2/build_pod.go	`98.65% <0.00%> (+0.04%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pkg/cnb/project_descriptor_test.go

pkg/cnb/project_descriptor.go

jjbustamante · 2022-09-02T15:25:38Z

I just added more unit testing to the merge logic. I agreed that pack has not merging logic. If we agree to avoid that one I can revert to my first commit.

@samj1912, @tylerphelan

sambhav · 2022-09-02T16:02:08Z

@jjbustamante what I meant by #1027 (comment) was that if both the fields are set, the new field should have higher precedence and we should ignore the old one. Apologies if it came across as merging the two. We should also have a test case similar to pack where we check that the env vars are from the new field and not the old one.

jjbustamante · 2022-09-02T16:19:09Z

@jjbustamante what I meant by #1027 (comment) was that if both the fields are set, the new field should have higher precedence and we should ignore the old one. Apologies if it came across as merging the two. We should also have a test case similar to pack where we check that the env vars are from the new field and not the old one.

Thanks @samj1912 for the clarification, I removed the merging code and the test case added was also modified. If you defined environment variables in the old version and the on the new version, kpack is going to take ONLY the one defines in the new version.

pkg/cnb/project_descriptor.go

sambhav · 2022-09-02T17:10:41Z

pkg/cnb/project_descriptor_test.go

+							ioutil.WriteFile(projectToml, []byte(`
+[_]
+schema-version = "0.2"
+[[io.buildpacks.env]]


@jjbustamante shouldn't this be io.buildpacks.env.build?

That one is defined in L293 see here

In that case, I defined the same variable twice:

schema-version = "0.2" [[io.buildpacks.env]] name = "keyA" value = "valueA" # check that new declaration must have higher precedence [[io.buildpacks.build.env]] name = "keyA" value = "newValueA"

And then in the assert, I verify the one taken was newValueA

@jjbustamante I meant to comment on the key itself. Left some notes at #1027 (review)

sambhav · 2022-09-02T17:10:56Z

pkg/cnb/project_descriptor_test.go

+							ioutil.WriteFile(projectToml, []byte(`
+[_]
+schema-version = "0.2"
+[[io.buildpacks.env]]


Suggested change

[[io.buildpacks.env]]

[[io.buildpacks.env.build]]

sambhav

The current 'old' code in kpack seems a bit incorrect? See output section of buildpacks/pack#1479

for the old key vs the new key

old pack key - io.buildpacks.env.build

new pack key - io.buildpacks.build.env

old kpack key - io.buildpacks.env? (this seems incorrect?)

jjbustamante · 2022-09-02T17:59:07Z

The current 'old' code in kpack seems a bit incorrect? See output section of buildpacks/pack#1479

for the old key vs the new key

old pack key - io.buildpacks.env.build

new pack key - io.buildpacks.build.env

old kpack key - io.buildpacks.env? (this seems incorrect?)

Oh! That's interesting. @matthewmcnew @tylerphelan @tomkennedy513 any thoughts on that? I didn't touch that part

tylerphelan · 2022-09-02T18:27:06Z

pkg/cnb/project_descriptor.go

+func getEnvVariables(d descriptorV2) []envVariable {
+	env := d.IO.Buildpacks.BuildEnv.Env
+	if env == nil {
+		env = d.IO.Buildpacks.Env


let's start a thread here per: #1027 (comment)

The current 'old' code in kpack seems a bit incorrect? See output section of buildpacks/pack#1479

It appears we incorrectly used io.buildpacks.env while pack incorrectly used io.buildpacks.env.build.

We can either support all three or ignore support for io.buildpacks.env.build

@samj1912 @tomkennedy513 @matthewmcnew @jjbustamante

Kpack exists in the pack ecosystem where users would reasonably expect kpack to behave similarly to pack. So, I think we should provide backwards compatibility to both io.buildpacks.env.build and kpack incorrect key: io.buildpacks.env.

Good 👀 @samj1912 !

sambhav · 2022-09-02T18:47:39Z

So IIUC

highest to lowest precedence -

io.buildpacks.build.env
io.buildpacks.env.build
io.buildpacks.env
?

jjbustamante · 2022-09-02T22:56:22Z

io.buildpacks.build.env

io.buildpacks.env.build

io.buildpacks.env

I agree with the major reason to do it like this, but, if I am not wrong, I am not able to use the same structs to deal with the 3 cases. The struct to handle build.env and env.build can be the same, with two fields but if I get an error trying to parse the toml file then I will have to create a different struct to try to parse the case env, right?

jjbustamante · 2022-09-02T23:44:43Z

I am not totally happy, but I did a refactor to include the compatibility for all the 3 cases:

io.buildpacks.build.env
io.buildpacks.env.build
io.buildpacks.env

If anyone have suggestions about the approach, let me know.

pkg/cnb/project_descriptor.go

pkg/cnb/env_vars.go

tomkennedy513 · 2022-09-07T20:02:04Z

@jjbustamante can you rebase this pr since its out of date

… also adding backwards compatibility with previous version Signed-off-by: Juan Bustamante <jbustamante@vmware.com>

jjbustamante · 2022-09-07T20:11:52Z

@jjbustamante can you rebase this pr since its out of date

Done @tomkennedy513
I also squashed the commits.

tomkennedy513 approved these changes Sep 1, 2022

View reviewed changes

jjbustamante marked this pull request as ready for review September 1, 2022 21:46

jjbustamante added this to the kpack 0.7.0 milestone Sep 1, 2022

sambhav reviewed Sep 1, 2022

View reviewed changes

pkg/cnb/project_descriptor_test.go Show resolved Hide resolved

tomkennedy513 requested review from matthewmcnew, chenbh, ncarlson, yaelharel and tylerphelan September 2, 2022 14:48

tylerphelan reviewed Sep 2, 2022

View reviewed changes

pkg/cnb/project_descriptor.go Outdated Show resolved Hide resolved

jjbustamante force-pushed the bugfix/issue-1017-support-correct-specification-of-project-descriptor branch from 9b07b40 to 3245661 Compare September 2, 2022 16:16

tomkennedy513 reviewed Sep 2, 2022

View reviewed changes

pkg/cnb/project_descriptor.go Outdated Show resolved Hide resolved

jjbustamante force-pushed the bugfix/issue-1017-support-correct-specification-of-project-descriptor branch from 3245661 to 0e3c15f Compare September 2, 2022 16:26

tomkennedy513 approved these changes Sep 2, 2022

View reviewed changes

ncarlson approved these changes Sep 2, 2022

View reviewed changes

sambhav reviewed Sep 2, 2022

View reviewed changes

sambhav requested changes Sep 2, 2022

View reviewed changes

tylerphelan reviewed Sep 2, 2022

View reviewed changes

jjbustamante force-pushed the bugfix/issue-1017-support-correct-specification-of-project-descriptor branch from e15e027 to e5ad236 Compare September 6, 2022 16:22

matthewmcnew reviewed Sep 6, 2022

View reviewed changes

pkg/cnb/project_descriptor.go Outdated Show resolved Hide resolved

matthewmcnew reviewed Sep 6, 2022

View reviewed changes

pkg/cnb/project_descriptor.go Outdated Show resolved Hide resolved

jjbustamante force-pushed the bugfix/issue-1017-support-correct-specification-of-project-descriptor branch from 5f13ba2 to ca3f513 Compare September 7, 2022 15:18

tylerphelan reviewed Sep 7, 2022

View reviewed changes

pkg/cnb/env_vars.go Show resolved Hide resolved

tomkennedy513 approved these changes Sep 7, 2022

View reviewed changes

matthewmcnew reviewed Sep 7, 2022

View reviewed changes

pkg/cnb/env_vars.go Outdated Show resolved Hide resolved

matthewmcnew approved these changes Sep 7, 2022

View reviewed changes

tylerphelan approved these changes Sep 7, 2022

View reviewed changes

Fixing project.toml build env to match specification changes in v0.2,…

37f948d

… also adding backwards compatibility with previous version Signed-off-by: Juan Bustamante <jbustamante@vmware.com>

jjbustamante force-pushed the bugfix/issue-1017-support-correct-specification-of-project-descriptor branch from bdb3abf to 37f948d Compare September 7, 2022 20:10

tomkennedy513 merged commit ad6c587 into buildpacks-community:main Sep 7, 2022

jjbustamante deleted the bugfix/issue-1017-support-correct-specification-of-project-descriptor branch September 7, 2022 20:26

jjbustamante self-assigned this Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support correct specification of project descriptor #1027

Support correct specification of project descriptor #1027

jjbustamante commented Sep 1, 2022

codecov-commenter commented Sep 1, 2022 •

edited

Loading

jjbustamante commented Sep 2, 2022

sambhav commented Sep 2, 2022

jjbustamante commented Sep 2, 2022

sambhav Sep 2, 2022

jjbustamante Sep 2, 2022

sambhav Sep 2, 2022

sambhav Sep 2, 2022

sambhav left a comment •

edited

Loading

jjbustamante commented Sep 2, 2022

tylerphelan Sep 2, 2022 •

edited

Loading

matthewmcnew Sep 2, 2022

matthewmcnew Sep 2, 2022

sambhav commented Sep 2, 2022

jjbustamante commented Sep 2, 2022

jjbustamante commented Sep 2, 2022

tomkennedy513 commented Sep 7, 2022

jjbustamante commented Sep 7, 2022

Support correct specification of project descriptor #1027

Support correct specification of project descriptor #1027

Conversation

jjbustamante commented Sep 1, 2022

Problem

Solution

codecov-commenter commented Sep 1, 2022 • edited Loading

Codecov Report

jjbustamante commented Sep 2, 2022

sambhav commented Sep 2, 2022

jjbustamante commented Sep 2, 2022

sambhav Sep 2, 2022

Choose a reason for hiding this comment

jjbustamante Sep 2, 2022

Choose a reason for hiding this comment

sambhav Sep 2, 2022

Choose a reason for hiding this comment

sambhav Sep 2, 2022

Choose a reason for hiding this comment

sambhav left a comment • edited Loading

Choose a reason for hiding this comment

jjbustamante commented Sep 2, 2022

tylerphelan Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

matthewmcnew Sep 2, 2022

Choose a reason for hiding this comment

matthewmcnew Sep 2, 2022

Choose a reason for hiding this comment

sambhav commented Sep 2, 2022

jjbustamante commented Sep 2, 2022

jjbustamante commented Sep 2, 2022

tomkennedy513 commented Sep 7, 2022

jjbustamante commented Sep 7, 2022

codecov-commenter commented Sep 1, 2022 •

edited

Loading

sambhav left a comment •

edited

Loading

tylerphelan Sep 2, 2022 •

edited

Loading