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.

Sign up for GitHub

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

Jump to bottom

Bump build api from v1alpha1 v1alpha2 #676

Merged
merged 1 commit into from
Jul 20, 2021
Merged

Bump build api from v1alpha1 v1alpha2 #676

merged 1 commit into from
Jul 20, 2021

Conversation

c0d1ngm0nk3y
Copy link
Contributor

This only introduces the api version v1alpha2 with no changes yet.

This is based on #675

This PR would be the common base for #555 and #669 which both introduce incompatible changes.

@c0d1ngm0nk3y
Copy link
Contributor Author

@matthewmcnew Could you have a look? This takes the "dumb" bumping of #555 and #669. So the PRs could rebase and focus on needed changes to the api. Could this be a way forward? WDYT?

@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the bump-build-v1alpha2 branch 2 times, most recently from bc2541f to 75ebbd7 Compare May 10, 2021 06:47
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2021 •
edited
Loading

Codecov Report

Merging #676 (73d4c18) into main (27ea452) will increase coverage by 0.87%.
The diff coverage is 73.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #676      +/-   ##
==========================================
+ Coverage   68.54%   69.42%   +0.87%     
==========================================
  Files          93      117      +24     
  Lines        4130     5069     +939     
==========================================
+ Hits         2831     3519     +688     
- Misses        951     1184     +233     
- Partials      348      366      +18
Impacted Files Coverage Δ
pkg/apis/build/v1alpha2/builder_lifecycle.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/builder_types.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/buildpack_metadata.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/buildpack_types.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/cluster_builder_types.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/cluster_stack_types.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/cluster_store_types.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/image_lifecycle.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/register.go 0.00% <0.00%> (ø)
pkg/apis/build/v1alpha2/source_resolver_types.go 0.00% <0.00%> (ø)
... and 77 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27ea452...73d4c18. Read the comment docs.

@modulo11
Copy link
Contributor

modulo11 commented Jun 7, 2021

Note: After 0.3.0 is released, we'll review all changes and copy them to the next api version.

@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the bump-build-v1alpha2 branch 2 times, most recently from 276e01e to 20026f8 Compare June 21, 2021 08:14
@c0d1ngm0nk3y c0d1ngm0nk3y marked this pull request as ready for review June 21, 2021 08:15
@c0d1ngm0nk3y
Copy link
Contributor Author

@matthewmcnew @tomkennedy513 As discussed in the last community meeting, this is the follow up on #675

@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the bump-build-v1alpha2 branch from 582022b to e3f2848 Compare June 24, 2021 08:32
@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the bump-build-v1alpha2 branch 2 times, most recently from bda567d to 530df24 Compare July 2, 2021 19:05
matthewmcnew
matthewmcnew reviewed Jul 7, 2021
View reviewed changes
pkg/apis/build/v1alpha2/build_pod.go

type stepModifier func(corev1.Container) corev1.Container

func (b *Build) BuildPod(images BuildPodImages, secrets []corev1.Secret, taints []corev1.Taint, config BuildPodBuilderConfig) (*corev1.Pod, error) {
Copy link
Collaborator

@matthewmcnew matthewmcnew Jul 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Should we delete the equivalent of these domain methods in v1alpha1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right I guess. I have removed them, but in a separate commit. So it is easier to revert again. I can squash it, once this is ready to merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we'd like to keep them to minimize the manual effort when doing a version bump. Also we should not touch old versions to not break existing code (maybe deprecate the methods instead).

@matthewmcnew
Copy link
Collaborator

I am a bit concerned about the proliferation of api versions and the added complexity introduced with multiple conversions/etc. I wonder if similar to tekton we should jump to v1beta1 or introduce v1alpha1 as unstable "testing" version before ultimately releasing it as a stable api in v1alpha1.

I'll add it to the agenda as this would be a great topic to rediscuss during working group.

@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the bump-build-v1alpha2 branch from 530df24 to 16e215c Compare July 8, 2021 11:13
@c0d1ngm0nk3y
Copy link
Contributor Author

I am a bit concerned about the proliferation of api versions and the added complexity introduced with multiple conversions/etc. I wonder if similar to tekton we should jump to v1beta1 or introduce v1alpha1 as unstable "testing" version before ultimately releasing it as a stable api in v1alpha1.

There is definitely some complexity coming with having more versions. But on the other hand, adding new features like #669 #555 or #670 and going from alpha to beta might contradict.

I'll add it to the agenda as this would be a great topic to rediscuss during working group.

Thanks.

@loewenstein
Copy link

I'd argue that alpha is the right stage to start practicing versioning of CRDs. Once something is v1, making mistakes with a potential successor version will be worse.

@matthewmcnew matthewmcnew mentioned this pull request Jul 12, 2021
Closed
@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the bump-build-v1alpha2 branch from 16e215c to bbd4fc6 Compare July 13, 2021 06:41
@modulo11 modulo11 force-pushed the bump-build-v1alpha2 branch from bbd4fc6 to 58d7679 Compare July 14, 2021 07:45
@modulo11 @c0d1ngm0nk3y
Create new buildapi v1alpha2
73d4c18 
- Bump Build CRDs (copy v1alpha1)
- Change buildapi import alias to v1alpha2
- Adapt documentation how to bump build api

Co-authored-by: Benjamin Haegenlaeuer <benjamin.haegenlaeuer@sap.com>
Co-authored-by: Ralf Pannemans <ralf.pannemans@sap.com>
Co-authored-by: Johannes Dillmann <j.dillmann@sap.com>
@modulo11 modulo11 force-pushed the bump-build-v1alpha2 branch from 58d7679 to 73d4c18 Compare July 14, 2021 07:46
@tomkennedy513 tomkennedy513 merged commit e34ea74 into buildpacks-community:main Jul 20, 2021
@matthewmcnew
Copy link
Collaborator

Thanks @c0d1ngm0nk3y!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@modulo11 modulo11 modulo11 left review comments

@tomkennedy513 tomkennedy513 tomkennedy513 approved these changes

@matthewmcnew matthewmcnew matthewmcnew approved these changes

Assignees
No one assigned
Labels
0.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@c0d1ngm0nk3y @codecov-commenter @modulo11 @matthewmcnew @loewenstein @tomkennedy513